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 ESM version #338

Merged
merged 2 commits into from
Apr 18, 2024
Merged

Add ESM version #338

merged 2 commits into from
Apr 18, 2024

Conversation

bashmish
Copy link
Contributor

Fixes #337

@tscanlin
Copy link
Owner

Hey, thanks for the PR! This looks good to me. How would this affect imports of other sub-dependencies for non-esm projects? I guess since utils wasn't touched it shouldn't be a big deal.

Thanks again!

@bashmish
Copy link
Contributor Author

bashmish commented Apr 15, 2024

Hey, thanks for the PR! This looks good to me. How would this affect imports of other sub-dependencies for non-esm projects? I guess since utils wasn't touched it shouldn't be a big deal.

Thanks again!

It shouldn't impact subdependencies for non-ESM, I didn't change the behavior for those.

Do you have some project where we can test if this works? I can make a prerelease version with @bashmish/tocbot for now to be able to test it on a real project.

Copy link
Owner

@tscanlin tscanlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a test for .cjs.js require style of use? I don't currently have a project that is using it aside from in the project itself. But running the local development server and requiring from there may help to test it.

@bashmish
Copy link
Contributor Author

bashmish commented Apr 16, 2024

Should there be a test for .cjs.js require style of use?

@tscanlin static/js/tocbot.js is generated with the input file of index-dist.js with a UMD wrapper and is injected like before via a script tag, so I thought that would be sufficient to test it like that, despite that test cases are loaded from a ESM. The latter is done, because it was easy to use such file to load in both ESM and UMD versions and reuse tests, it's only the tests cases which are loaded like that, the initialisation of the library remained the same I think for UMD, and for ESM there is a separate new initialisation file. Maybe I didn't get some nuances in how UMD was tested before, would be nice if you can elaborate.

I don't currently have a project that is using it aside from in the project itself. But running the local development server and requiring from there may help to test it.

What particular way of loading do you want to test?
I used ESM and a simple script. ESM is standard, injecting a script via a script tag and exposing on the window.tocbot is also standard (this was the way it was tested before my PR), no special tools are involved. Then there would be more ways to load it using different tools, with a spectrum between old-school require.js and new-school like Vite.

@tscanlin tscanlin merged commit fbfa1ec into tscanlin:master Apr 18, 2024
4 checks passed
@tscanlin
Copy link
Owner

Thanks for the PR!!

@tscanlin
Copy link
Owner

tocbot@4.27.0 is published with this change. Thanks @bashmish!

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.

ESM version
2 participants