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
(feat) include ESM in CDN build #3209
Conversation
tools/build_cdn.js
Outdated
await buildPackageJSON(options); | ||
const json = require(`${process.env.BUILD_DIR}/package`); | ||
const json = buildPackageJSON(options, { | ||
".": { |
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.
Why would this matter for web usage?
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 see now that the npm exports
has been drug along into the v11 CDN package unintentionally, but I'm not sure this is necessary at all. There are two common uses cases for CDN:
- Purely a delivery system to get our files onto CDNs, where they are directly loaded/imported (most common)
- Via
npm
when someone really wants JUST the build assets to directly copy into their project
I'm not sure exports
is necessary for either of these cases.
Thoughts?
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 don't think it's necessary indeed, but I thought it wouldn't hurt to keep it I think. It could be helpful for an hypothetical future tool that can generate an import map from a package.json
"exports"
, and humans can also read it to understand the structure of the files to understand which url they should import.
I believe we'd still want a common build - and I think it would be a monolith (ie, languages actually compiled in)... someone just wanting the "default it just works" setup but also wanting ESM should still be able to have a single source to import to get started, etc... |
Also currently failing tests. |
Doesn't look like there is any way to test ESM web builds with jsdom? |
@oscarotero Could you pull this, do a |
@joshgoebel I've added tests for Deno to test the ESM build. I'm not suggesting we should merge it, but I thought it could be interesting as a POC. wdyt? |
@joshgoebel For convenience for Deno developers, I recomend not to minify the code, so it's more easy to see the file and line of possible errors. |
My question about testing was specifically related to a browser-like environment, not Deno per se. Yes, it's obvious we could perhaps add a few Deno tests... but let's have that discussion elsewhere (perhaps in the new issue I just created). Could you please open a second PR with those last few changes and link it to the new issue? |
I actually need the changes from this PR to run the Deno test (because it needs plain ESM to work), should I remove it from this PR and open a new one when/if this one lands, or should I leave it in (after, it tests that the ESM output is somewhat valid so I suppose that's valuable). |
Yes, you'd just push your changes as-they-were to another branch. I wasn't suggesting start form scratch on I don't think we need to do this any longer though... as I don't think these are good baseline tests for Deno as they don't prove anything truly useful. They might serve as regression tests for the API, but they don't provide any real confidence that Deno has the same behavior as say Node.js since they don't exhaustively test the engine (or really test it much at all). If Deno is compatible in the least then we'd expect basic function dispatch to work.
I don't believe these Deno tests are a desirable way to "test ESM output in general" either... that would be better done by trying a sample bundle/build with ESM during our CI as we do already to test some bundler edge cases. The only reason to add Deno specific tests at this juncture is if they provide value in pushing us towards perhaps officially supporting Deno in the future. As I mentioned on the other thread if the desire is to move towards "Deno is officially supported" then we should start with porting |
To be clear there's nothing wrong with the tests per se, I just don't feel they have value on their own right now... if you wanted to go a step further and add the markup tests as well then the API tests would definitely be great to have in addition... as a regression check for the API on Deno... but just not as our only Deno tests. |
Thanks for the clarification, I'll open a draft PR with the Deno tests after this one lands. |
This still seems broken when a language uses
It also breaks the ESM build since it now includes Previously this was dealt with since we compiled to IIFE first (which had the effect of normalizing this)... now that you are directly compiling to ES it seems to just leave |
@joshgoebel where do you see this $ yarn build-cdn
✨ Done in 8.57s.
$ grep -r 'module.exports' build/
build//highlight.js:if (typeof exports === 'object' && typeof module !== 'undefined') { module.exports = hljs; }
build//highlight.min.js:;"object"==typeof exports&&"undefined"!=typeof module&&(module.exports=hljs); |
Just add a 3rd party module that uses |
I didn't know about extra languages, I'm surprised this didn't come up in earlier discussions about switching to ESM. In a separate discussion, we might want to discuss deprecating the use of CJS for those maybe? In any case, I've added support for detection of the I've also added support for |
Didn't think of it. Of course 3rd party languages go straight into the build pipeline just like 1st party ones so it should have "just worked" had you not also changed the input processing in what I presume was an effort to improve it.
I'm in no hurry to deprecate CJS - and still think it's the simplest way to get started for many of our users, but I've added an
This is complexity we do not need at the present time and also I don't think guaranteed to be backwards compatible. We also have to support support grammars with no I've reverted back to the much simpler IIFE build process we used before. ESM modules are just IIFE + added export at the end. Since variables should be file local I think this works, am I mistaken?
|
I'm also not sure becoming further tangled with NPM in this way is a step in the right direction. It would be much simpler for us to drop
I don't find this an onerous restriction, and of course someone is always free to use their own build process. BUT since what we do now (supporting |
Good work on simplifying the IIFE build process, I think I had some kind of misconception about how the CJS plugin was working, I ended up doing much more work than necessary, oh well.
Fair enough, I'd argue that Let me know if there's something more for me to do in this PR. |
@@ -125,7 +125,7 @@ async function installLanguages(languages, options) { | |||
|
|||
await Promise.all( | |||
languages.filter((l) => l.third_party) | |||
.map((lang) => buildDistributable(lang, options)) | |||
.map(async(lang) => await buildDistributable(lang, options)) |
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.
Out of curiosity, what's the reason for using async/await here?
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.
Uncertainty about how async works? :-) Are you going to tell me it's effectively the same either way?
Sorry for the back and forth, but I think it's more efficient to ask rollup to output an ES module here: Before: var hljsGrammar=(()=>{"use strict";return e=>{
/*...*/}})();export default hljsGrammar; After: function ada(e){
/*...*/}export default ada; It saves a few bytes (which is always nice for a CDN build), and produces the exact same output for the IIFE files. |
- also build an ESM module in `extra/grammar/dist`
Maybe but I've kind of hand verified things the old way (before your last commit) and getting kind of burnt out on this PR... so we'll go with it as it is and then we can circle back in the future. What you're suggesting though is the REAL problem before was the use/lack of use of plugins when you switched to ESM, not the actual switch from IIFE to ESM that was breaking the default.exports? |
Yes that's exactly it, I had removed the plugins because I didn't know about extra languages written in CJS.
No problem, take care! |
Thanks for all your work on the PR and providing momentum to help get this done. 👍 I plan to push a release this week I think. |
Currently, this PR doesn't include an ES module pre-compiled with common languages. ESM user need to load both
core
and each individual language module. Should I change this?Fixes: #3199
Changes
Checklist
CHANGES.md