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

misc: migrate all packages to ESM-only #5816

Closed
wants to merge 16 commits into from
Closed

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented Oct 29, 2021

Breaking changes

  • For plugin authors or others who write server-side code that imports from Docusaurus packages: please migrate your code to ESM as well in order to continue importing our code. We will help migrate a few widely used community packages.

Motivation

Resolve #5379. Since we are now Node 14+, we can work on migrating our packages to ESM to be in sync with the JS community, especially Remark/Rehype.

The current TS tranpilation is not good enough so E2E tests are failing. From the release notes of TS 4.5, there would be prioritized support for ESM, so we can wait to upgrade TS.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

E2E tests, local dev preview

@Josh-Cena Josh-Cena added status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. pr: maintenance This PR does not produce any behavior differences to end users when upgrading. labels Oct 29, 2021
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 29, 2021
@Josh-Cena Josh-Cena added this to the 2.0.0 milestone Oct 29, 2021
@slorber
Copy link
Collaborator

slorber commented Oct 29, 2021

great 👍

isn't the problem in copyUntypedFiles.js ?

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Oct 29, 2021

isn't the problem in copyUntypedFiles.js?

Man, it's far more than that😅 If it's just require => import I would've done it already

With ESM:

  • You have to always specify the .js extension
  • You have to explicitly say index.js when you are trying to import a directory

These are to make the behavior exactly like the browser behavior. There's a node flag --experimental-specifier-resolution=node to restore the previous behavior, but I've been unable to get it working. we may have to use the NODE_OPTIONS in the executable's hashbang. I'll look at that later, but with the prospected TS 4.5 migration, I'd wait a bit more

@Josh-Cena Josh-Cena added pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. and removed pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. labels Oct 30, 2021
@Josh-Cena
Copy link
Collaborator Author

It's almost working! Currently blocked by import-fresh which uses CJS require. Seems the ESM support is stalled: sindresorhus/import-fresh#22

@slorber
Copy link
Collaborator

slorber commented Nov 3, 2021

FYI we'd also need to require Node >= 14.14, not just >= 14

https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

@Josh-Cena
Copy link
Collaborator Author

When I check for features it doesn't seem 14.5 and 14.18 have a big difference besides some class fields stuff... I guess that Sindre Sorhus gist was just playing it safe by recommending the last minor version (while 14.14 is not the last minor version now)? 12.20 is also in the list, so I guess it would be fine. We have CI checks anyways

@slorber
Copy link
Collaborator

slorber commented Nov 3, 2021

We have CI checks anyways

CI checks are "14", I suspect it's different than "14.0". We'll figure out later what min version to recommend ;)

@Josh-Cena Josh-Cena changed the title chore: migrate all packages to ESM-only misc: migrate all packages to ESM-only Nov 5, 2021
@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Nov 5, 2021

@slorber what do we use import-fresh to achieve anyways? Is it used for watch mode?

@slorber
Copy link
Collaborator

slorber commented Nov 5, 2021

importFresh(configPath) ensures notably that when file watchers trigger a site reload, imported JS modules (like config) are not cached and we actually load a fresh config, including the recent updates

@Josh-Cena
Copy link
Collaborator Author

I see... I saw somewhere that a common practice is to add unique query strings to each import call in the ESM world, but I also saw there's no proper GC in this case and risks OOM

@slorber
Copy link
Collaborator

slorber commented Nov 5, 2021

Yes, the former cached modules will likely stay in the cache forever.

Isn't it possible to keep using importFresh and mixing ESM with some CJS here and there?

Or will we have to ask our users to convert their config/sidebars to ESM too?

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Nov 5, 2021

Isn't it possible to keep using importFresh and mixing ESM with some CJS here and there?

Or will we have to ask our users to convert their config/sidebars to ESM too?

The interoperability of CJS and ESM is like:

  • require of ESM always fails (because Node wants you to upgrade to use new features);
  • import of CJS usually works (because Node has to support those ancient packages), but sometimes resolving named imports can be wonky because they have to be statically determined pre-runtime

Config files can stay as CJS as long as they don't try to require any Docusaurus package. I don't know if it would always work; worth trying. importFresh would never work because we use it to load plugins, which are now ESM-only.

@Josh-Cena
Copy link
Collaborator Author

There's an interesting suggestion of using worker threads to import fresh modules by killing the worker thread👀 I think workers can offer a new perspective of how Docusaurus works, but not sure if we want such architectural changes.

@Josh-Cena Josh-Cena mentioned this pull request Nov 14, 2021
@@ -1,17 +1,19 @@
#!/usr/bin/env node
#!/usr/bin/env node --experimental-specifier-resolution=node
Copy link
Contributor

Choose a reason for hiding this comment

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

Some day, this flag is may be removed or renamed and completely break everything. I feel like it's not a good idea to hard-code it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this is just to make sure things work. Soon after things start working we will try to explicitly specify file names & extensions (especially when TS 4.6 has better support for this)

@RDIL
Copy link
Contributor

RDIL commented Nov 29, 2021

I like the direction this is going in, but I really don't think ESM is near ready enough.

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Nov 29, 2021

I like the direction this is going in, but I really don't think ESM is near ready enough.

Well we gotta make this move. MDX v2 is ESM-only, so we either have to build it as CJS and re-publish it, or we stay on v1 forever. A lot of remark/rehype plugins are ESM only as well, and ESM cascades down the require chain. Ultimately you just want the entire code base to be ESM.

@Josh-Cena
Copy link
Collaborator Author

Closing because of too many conflicts. We will revisit this in the future, and probably do it progressively (e.g. migrating tests first, then core, then plugins, then utils)

@Josh-Cena Josh-Cena deleted the jc/esm-transpilation branch January 30, 2022 05:41
@Josh-Cena Josh-Cena mentioned this pull request Jan 31, 2022
1 task
@slorber slorber modified the milestones: 3.x+, 3.0 Aug 17, 2023
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: breaking change Existing sites may not build successfully in the new version. Description contains more details. pr: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ESM config file
4 participants