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

Provide CJS build #12

Closed
brettz9 opened this issue Jun 23, 2024 · 10 comments · Fixed by #13
Closed

Provide CJS build #12

brettz9 opened this issue Jun 23, 2024 · 10 comments · Fixed by #13

Comments

@brettz9
Copy link

brettz9 commented Jun 23, 2024

While it worked for us locally, as we are also using modules, I dumbly forgot to check whether the package supports CJS.

Any chance you'd provide a CJS build?

@TomerAberbach
Copy link
Owner

Looks like my two dependencies both provide CJS builds so should be possible

@TomerAberbach
Copy link
Owner

@brettz9 are you by any chance able to clone the #13 branch, run install, run npm run build, and confirm that using the package locally works for you CJS-wise?

@brettz9
Copy link
Author

brettz9 commented Jun 23, 2024

I had problems doing it locally (trying npm link with transitive dependencies), but instead I've made a fork which includes the built files (removing dist from .gitignore). It seems the one thing you will still need to do in your PR is remove "files": ["src"] from package.json (or tweak it to include dist). The package then works as expected in either an ESM or CJS environment.

Unfortunately, in doing testing this way, the race condition occurs and I get the error, "Expected WASM to be loaded before calling parseImportsSync", so it seems we'll be unable to use the package after all. :-( Thanks for your efforts though!

@TomerAberbach
Copy link
Owner

TomerAberbach commented Jun 23, 2024

Ah good catch with the "files": ["src"]!

Up to you whether it's worth the trouble, but you could theoretically run this package's async method in a worker to make it synchronous. For example: eslint/eslint#15394 (comment)

If you are interested in that, then I can move forward with this CJS export

@TomerAberbach
Copy link
Owner

TomerAberbach commented Jun 23, 2024

One other similar option: https://github.com/sindresorhus/make-synchronous. I think this one uses a child process instead of a worker, so probably not as efficient as the other option I mentioned

@brettz9
Copy link
Author

brettz9 commented Jun 24, 2024

Thanks for that!

While make-synchronous gave me an error (Cannot find package 'subsume' imported from /Users/brett/eslint-plugin-jsdoc/[eval1] for posterity), synckit did the trick (at least with the worker having and referenced with an .mjs extension).

So a CJS release would be great.

@TomerAberbach
Copy link
Owner

Nice! Glad that works. Will publish the version with CJS soon

@TomerAberbach
Copy link
Owner

Published as v2.1.0!

@brettz9
Copy link
Author

brettz9 commented Jun 24, 2024

Excellent, thanks! Love the turnaround speed too! :-)

@TomerAberbach
Copy link
Owner

Excellent, thanks!

No problem!

Love the turnaround speed too! :-)

Haha, have been avoiding going outside due to the heat here in the Northeast 😅

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 a pull request may close this issue.

2 participants