Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional body field schema generic to N/record method types #299

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ahowley
Copy link
Contributor

@ahowley ahowley commented Jul 27, 2024

As it is now, record.getValue() always returns the FieldValue union type, and record.setValue() accepts a FieldValue union type. This often requires type casting each time you getValue() and need to use it as a specific type. setValue has a (slightly) more dangerous problem: you can set a field to the wrong type and you'll only find out at runtime, removing some of the benefit of using types to begin with.

These changes aim to add optional generics - that is, without causing breaking changes - to allow users to pass a single custom "schema" type that maps NetSuite field IDs to their JavaScript types, allowing for type inference to detect improper field types at compile time. The user still needs to know the correct types of each field, but they now only need to define those relationships in one place.

At least one downside is that method signatures are uglier and less intuitive, so I'm open to feedback and discussion. Sublist and subrecord methods haven't been handled yet, so this only targets a record's body fields, for now. A new example-Record-generics.ts is included to demonstrate usage.

ahowley and others added 4 commits July 26, 2024 20:43
This adds optional field mapping via a schema generic to remaining
relevant methods - that is, any method which returns the same record as
the record calling the method, or any method that creates or loads a
record. Sublist and subrecord methods are not effected, except those
that return the parent record.
@ShawnTalbert
Copy link
Contributor

I cordially invite you to take a peek at netsuite-fasttrack-toolkit-ss2 (NFT)

@ahowley
Copy link
Contributor Author

ahowley commented Jul 27, 2024

NFT is useful, but it adds abstraction and means you're no longer using the native NetSuite APIs. Obviously, nothing wrong with that conceptually, but I figure strong typing on SuiteScript itself should warrant consideration. Especially when migrating from JavaScript code, which, at least for me, is a pretty common use case

If it's out of scope of this package that's fine, of course. But I do think the two achieve different things?

@ShawnTalbert
Copy link
Contributor

I do think this is a good addition, but I'm also biased as a contributor to NFT (where records are entirely strongly typed by design).

On the other hand, IMHO, doing this isn't really 'plain SuiteScript' any more. SuiteScript doesn't have native typing/constraints on field access. For example, vanilla SuiteScript is fine with you asking for or setting fields that don't even exist. I imagine that's one of the motivators for you to add this in the first place :)

Therefore, I do think it important that this typing be entirely optional (i.e. fully backwards compatible).
It looks like you've illustrated that in the example file but would be curious how this feels from the IDE with code completion - do the overloads look scary to users not as familiar with generics?

@ahowley
Copy link
Contributor Author

ahowley commented Oct 12, 2024

I've monkey patched the change into a few of my current projects to try it out, I can give a sense based on my experience thus far!

Since the generic schemas only need to be passed once (per record, in load(), create(), getCurrentRecord(), etc.), the code itself is mostly unchanged, except that there's no need to cast getValue(). I've caused fewer runtime errors from mismatched types or typos, and the autocompletion makes the whole SuiteScript interface feel a lot more stable to work with, since you handle the mental overhead of remembering which types are returned/expected just once, in the record schema types.

If type hints are turned on, it definitely does make them a little busier, though. Same with LSP hovers and TypeScript error messages (albeit most of the time, these are errors that would be hidden without types). As is seemingly always the case with SuiteScript (😭), this problem is worsened by sublists and subrecords.

Having worked with it for a few months, I think my pull request here might be a bit undercooked; I still think this idea could be worth pursuing, but without N/query and N/search (and Client entry point contexts, Suitelet parameters, etc.) being typed as well, and without utility types/aliases to "remember" the quirks for you (like how IDs are strings in loaded records but numbers in SuiteQL), it leaves enough to be desired that I have my doubts people would actually use it.

There is also a middle-ground option to leave the existing code untouched, and simply add TypedRecord<> and/or TypedCurrentRecord<> as module exports from N/types; this way, you could still get the benefits from only having to cast a record's fields in one place, but it's entirely opt-in, and easier to add to incrementally?

@ShawnTalbert
Copy link
Contributor

Frankly, I simply wouldn't be writing NetSuite scripts without NFT (or something equal/better). Since it raises strong typing as a first-class citizen in SuiteScript and does so uniformly across records/subrecords/sublists. It allows me to write generally idiomatic TypeScript and so I am not looking back :)

Hence my only objection would be if changes here needlessly broke existing (untyped) behavior.

Anything y'all want to do around a TypedRecord or similar 'opt-in' approaches could be certainly helpful for anyone that doesn't want to do NFT or similar.

@MrRob
Copy link
Collaborator

MrRob commented Oct 29, 2024

I'd like to understand these options better but I have a hard time from just reading about it. Any chance you could do a 3-5 minute Loom video walking through what's involved here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants