-
-
Notifications
You must be signed in to change notification settings - Fork 457
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
chore(workspace): automatically publish urql-core to JSR #3574
Conversation
|
@@ -23,15 +23,21 @@ const SOURCE_NAME = 'gql'; | |||
const GRAPHQL_STRING_RE = /("{3}[\s\S]*"{3}|"(?:\\.|[^"])*")/g; | |||
const REPLACE_CHAR_RE = /(?:#[^\n\r]+)?(?:[\r\n]+|$)/g; | |||
|
|||
const replaceOutsideStrings = (str: string, idx: number) => | |||
const replaceOutsideStrings = (str: string, idx: number): string => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, and annotations like Map
below, seem a little concerning to me.
These are all internal functions and this seems to imply to me that JSR is enforcing a rule based on publishing raw TypeScript.
This may mean that we'd have to maintain a lot of changes across the code base for JSR and make a huge amount of changes. I think this would be reasonable if JSR/Deno would be the more popular platform over Node/npm, but it isn't.
We can implement an eslint rule for this (and there may even be an auto fixing one), but assuming declaration support is present in Deno, and we can publish prebuilts to JSR, wouldn't that be an easier way forward here?
If JSR doesn't allow this though, I think this seems a tad aggressive. Isolated declaration-like changes make sense to me, but changing all annotations to be statically analysable (except for functions where no assignment type is fine, but no return type isn't) seems a little aggressive 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've shared that feedback with them, not much more I can do. We can ofcourse publish the bundled files instead that's true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha! If we can't publish declarations yet, let's maybe publish this one package then figure out whether we want to do these changes for all packages.
Don't want to block this, but also want to make sure we don't have to make wide ranging changes across all packages of course 💙
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So to handle publishing JS we have to add a reference to the JS file
- Point the exports path to the js file
- Add a /// <reference path="my-script.d.ts" /> line at the top of the JS file to include the types
Summary
This adds a publishing pipeline to jsr, for now it will only publish
@urql/core
because we have a lot of "slow types" where it doesn't infer from the generic or is implicit. I'd solve these one by one rather than creating one big PR. The PR already adds the infrastructure in each workspace to publish to JSR by means of thejsr.json
file. This also adds a new script during versioning where it will bump all thejsr
packages.A point for improvement that I'm considering in scope of this PR is a transformation script for our TS codebase where we transform all the
process.env.NODE_ENV
toimport.meta.env.NODE_ENV
or we strip it out. Not doing so will cut out deno support for a while, supporting both Deno as well as Node in JSR would requireglobalThis.Deno?.env.get("NODE_ENV") || process.env.NODE_ENV
. I think with regards to graphql/graphql-js#4075 it might be a good idea to transform our ESM bundles toimport.meta.env
sometime either way.Currently we do not support canaries on jsr.
TODO