Proposal: improve TS experience when dealing with NodeJS APIs#295
Draft
ernest-nowacki wants to merge 1 commit intomainfrom
Draft
Proposal: improve TS experience when dealing with NodeJS APIs#295ernest-nowacki wants to merge 1 commit intomainfrom
ernest-nowacki wants to merge 1 commit intomainfrom
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is a proposal for improving DX experience when working with TS workflows and using unsupported APIs.
Context
Users are often thrown off guard and are presented with unreadable WASM traces when they use some of the NodeJS APIs that are not available in our Javy/QuickJS env. We would now change it to more readabile errors like this one:
That's a new compile-time check looking for imports of APIs like
node:crypto,node:httpetc.We're also making changes to default
tsconfig.jsonto NOT INCLUDE by default NodeJS types. What is being picked up however is our own custom types, marking APIs asneverand deprecated. What it does in practice?When you hover over imports in your IDE you would see suggestion to use our capabilities:
If you still decide to use particular API in the code itself you would see it crossed out:
If you run your
bun typecheckyou would also see TS errors:It should give you a decent understanding of what's wrong compared to current experience.
How to test
New workflows
bun typecheckto verify types in your workflow.For pre-existing workflows:
package.jsonto point tocre-sdk@1.1.3-alpha.2.types: []incompilerOptionsin your workflowtsconfig.json.bun install.Required
Next steps
This is just first step towards better use of TS for checking workflows. If this proposal is accepted we will continue making it better by exploring options to:
bun typecheckwe could make it required for the simulate / deploy taskscrypto-sha256which under the hood uses restricted API we would like to catch that toocrypto.randomBytes(16)), it would be better if we could error out as soon asimport { crypto } from 'node:crypto'