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

fix(core): make generated files have unambiguous module types #7379

Closed
wants to merge 2 commits into from

Conversation

Josh-Cena
Copy link
Collaborator

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Related: #6520

I set up a new website and added "type": "module" to the root package.json. Unsurprisingly, the build failed with a multitude of reasons. Some of them are related to docusaurus.config.js and sidebars.js being seen as ES modules, which will be fixed in #7371. However, there are some generated files that use require but are seen as ES modules, therefore triggering "require is not defined in ES module scope" error. To fix this, we simply need to make them .cjs, and use CJS-exclusive APIs instead. This is recommended practice because it makes the module type unambiguous.

In addition, I've added .cjs to the resolvable extensions list.

Test Plan

I applied these changes to a locally set up site and it works. The same changes should be reflected in the deploy preview as well.

When locally serving a production build, I noticed a flash of "cannot find module" banner from the serve handler. We will need to check that.

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

@Josh-Cena Josh-Cena added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label May 9, 2022
@Josh-Cena Josh-Cena requested review from slorber and lex111 as code owners May 9, 2022 05:06
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 9, 2022
@Josh-Cena Josh-Cena mentioned this pull request May 9, 2022
1 task
@netlify
Copy link

netlify bot commented May 9, 2022

[V2]

Name Link
🔨 Latest commit d79e8f9
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/627f62ecc88ac200096639fb
😎 Deploy Preview https://deploy-preview-7379--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented May 9, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 58 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 83 🟢 99 🟢 100 🟢 100 🟢 90 Report

@github-actions
Copy link

github-actions bot commented May 9, 2022

Size Change: -32.7 kB (-4%)

Total Size: 780 kB

Filename Size Change
website/build/assets/js/main.********.js 615 kB +38 B (0%)
website/build/index.html 6.22 kB -32.7 kB (-84%) 🏆
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 52.3 kB
website/build/assets/css/styles.********.css 106 kB

compressed-size-action

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented May 9, 2022

Oh, I think I hit the same issue as in #7238 (comment) 😭 I don't know how to fix this... The server build can't find the assets as soon as you do non-trivial changes to the module system... What's interesting here is that the server build doesn't fail, it only silently outputs a "module not found" banner... I don't know what's causing that behavior It's because the theme-fallback/Loading component catches the error and displays the error banner. Maybe we should throw the error up during SSR, so that it becomes visible? This is almost impossible to happen in userland, though.

@Josh-Cena Josh-Cena marked this pull request as draft May 9, 2022 05:25
@Josh-Cena
Copy link
Collaborator Author

OK, this is caused by the same problem as #7238 (comment), but in a different way. Because we need babel-plugin-dynamic-import-node to transform dynamic imports to require, so that Webpack does not code-split, we need to include .cjs extension in the JS loader regex as well. I forgot about that.

@Josh-Cena Josh-Cena force-pushed the jc/cjs-generated branch 2 times, most recently from 9855a3f to 91952c1 Compare May 14, 2022 08:59
@slorber
Copy link
Collaborator

slorber commented Sep 25, 2023

Closing for now but feel free to reopen

@slorber slorber closed this Sep 25, 2023
@Josh-Cena Josh-Cena deleted the jc/cjs-generated branch September 25, 2023 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants