-
-
Notifications
You must be signed in to change notification settings - Fork 134
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
browser package should be a module #237
Conversation
To be honest I don't really know what I'm doing here. I just fixed the errors I got. |
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.
@bomgar Thank you for the submission. Just a few small tweaks and I'd say this is ready to go. Thank you for including the changes to the test configs.
@MasterKale i did change the file to |
@MasterKale I changed the server to type |
I did a before and after build of server after adding However it appears adding that breaks projects that use
I wonder now if it's worth trying to preserve the deduplication of the Jest config... Tell you what, can you actually get rid of jest.config.mjs, and just copy its three config lines into the browser and server jest configs? Then forget about adding |
This PR updates the server package to It is a good idea, to convert the package to pure ESM. Therefore the An alternative approach would be to produce both an ESM bundle and a CJS bundle and use conditional exports. esbuild could be used instead of rollup to do the job. I have a similar build pipeline here for reference: https://github.com/P4sca1/cron-schedule/tree/v3.0.6. Personally, I would go with the ESM only approach, as it is easier to setup and maintain. This is also what other libraries did in the past - it is time to move away from CJS to ESM. This should then be a major release, which should maybe include the gist I referenced earlier. |
Server currently works fine in both ESM and CJS codebases, so I don't immediately see the need to make any changes to it. That's why I mention forgetting about adding We can explore the possibility of refactoring server's build but it's definitely outside the scope of this PR. |
@MasterKale I made the adjustments. |
Thank you for your contribution, @bomgar! |
fixes #236