Skip to content

Conversation

@thegreatercurve
Copy link
Contributor

@thegreatercurve thegreatercurve commented Apr 4, 2024

This should fix a couple of bugs which are affecting our internal sync:

  • As of Node fork modules & moduleResolution bundler #5774, the internal sync is broken as we're now loading the additional Prism language modules whereas before we weren't (probably erroneously), as we have our own versions of these packages.
    • This is causing a "Prism is undefined" error when we try to build the internal apps with Haste - I'm not 100% sure why but there's an issue with bundling named ESM versions of CommonJS exports.
    • We can fix this though by reverting to the unnamed ESM imports.
  • We probably do want to include the additional Prism language modules when building for internal, so I've disabled tree shaking for Lexical Code usage in all cases, including internal.

Test plan

Ensure that additional Prism modules are still included in LexicalCode.mjs and LexicalCode.prod.js build by running npm run build.

Ensure that additional Prism modules are now included in WWW build file LexicalCode.prod.js by running npm run build-www.

Run an draft internal sync and ensure that everything is working correctly.

@vercel
Copy link

vercel bot commented Apr 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 5, 2024 5:06pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 5, 2024 5:06pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 4, 2024
@github-actions
Copy link

github-actions bot commented Apr 4, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/lexical/dist/Lexical.js 26.77 KB (0%) 536 ms (0%) 77 ms (+266.63% 🔺) 612 ms
packages/lexical-rich-text/dist/LexicalRichText.js 39.14 KB (0%) 783 ms (0%) 105 ms (+83.53% 🔺) 887 ms
packages/lexical-plain-text/dist/LexicalPlainText.js 39.11 KB (0%) 783 ms (0%) 114 ms (+79.25% 🔺) 896 ms

@etrepum
Copy link
Collaborator

etrepum commented Apr 5, 2024

For what it's worth, other bundlers may tree-shake these out entirely since they're just imports that don't look like they have side-effects. Also setting "sideEffects": true in the @lexical/code package.json might resolve that, but would need some more testing across at least vite/esbuild and webpack.

I'd be happy to help sort this out, but I don't think there's a way for me to see what haste(?) does for the internal builds.

@thegreatercurve thegreatercurve changed the title Re-add tree-shakeable Prism imports Fix broken internal build due to new Prism imports Apr 5, 2024
@thegreatercurve thegreatercurve changed the title Fix broken internal build due to new Prism imports [WIP] Fix broken internal build due to new Prism imports Apr 5, 2024
@thegreatercurve thegreatercurve changed the title [WIP] Fix broken internal build due to new Prism imports Use unnamed ESM imports for Prism modules Apr 5, 2024
@thegreatercurve
Copy link
Contributor Author

For what it's worth, other bundlers may tree-shake these out entirely since they're just imports that don't look like they have side-effects. Also setting "sideEffects": true in the @lexical/code package.json might resolve that, but would need some more testing across at least vite/esbuild and webpack.

I'd be happy to help sort this out, but I don't think there's a way for me to see what haste(?) does for the internal builds.

@etrepum FWIW, thanks for taking a look into this, originally! Our internal build is actually handled within this repo, using the same Rollup config as the NPM build, minus a few differences using the isWWW flag in build.js (our server is called WWW).

It seems we were originally tree-shaking out the additional language modules for Prism in our Rollup config, but that's probably incorrect as we should include them internally

I'm not sure how external third-party bundlers handle unnamed ESM imports for tree-shaking, but we've always bundled the Prism plugins this way for NPM, and I don't think we've not had any reports about languages being missing from Prism, so generally, I think we should be okay to retain them.

@thegreatercurve thegreatercurve marked this pull request as ready for review April 5, 2024 17:19

export const getCodeLanguages = (): Array<string> =>
Object.keys(Prism.languages)
Object.keys(window.Prism.languages)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we expect any impact to OSS consumers from changing this to global?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Prism itself sets this as a window global which is how the languages register themselves on import, so I don't think so (unless they are calling it SSR without a window available somehow)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mostly meant that getCodeLanguages() could throw on the server since there's not necessarily a window in scope here, if someone set global.window = global then it would be fine 🤷 Probably just a theoretical issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably just a theoretical issue.

I just faced it in nextjs with headless editor, global.window = global works fine though. (2wheeh/lexical-nextjs-ssr@5f1691d)

Copy link
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

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

✅ I can confirm that this does the right thing in the environment I was able to reproduce the problem with before (an astro build). It also works correctly with next.js (when rebased on top of master to get the package.json fix), and the imports all seem to be present when manually inspecting the dev/prod output files.

@thegreatercurve
Copy link
Contributor Author

thegreatercurve commented Apr 5, 2024

Thanks @etrepum for confirming this still works on Astro and Next! Much appreciated.

@david10sing
Copy link

@thegreatercurve

In remix, I am getting Prism is undefined...

The side-effect import means it will run all the import definition.

In my project, I now seeing Prism loaded on the window context in Development when I actually do not need it.

During a production build, prism does not actually get included for some reason, resulting in the Prism is undefined in production...

However, the other imports like import 'prismjs/components/prism-clike'; are also being run but Prism is not set on window.

@moy2010
Copy link
Contributor

moy2010 commented Jun 25, 2024

@thegreatercurve

In remix, I am getting Prism is undefined...

The side-effect import means it will run all the import definition.

In my project, I now seeing Prism loaded on the window context in Development when I actually do not need it.

During a production build, prism does not actually get included for some reason, resulting in the Prism is undefined in production...

However, the other imports like import 'prismjs/components/prism-clike'; are also being run but Prism is not set on window.

I'm also facing the same issue with Next.js (14.2.3). I've tried to manually assign Prism to the window object (window.Prism = Prism), but then I get the error Cannot set properties of undefined (setting 'triple-quoted-string').

This only happens when building for production, though. It works as expected in dev.

Edit: Fixed it. It turns out that I had to import prism-java before prism-scala (the order of imports matters!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants