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.

Sign up for GitHub

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

Jump to bottom

refactor: convert CLI entry points to ESM; migrate create-docusaurus to ESM #6661

Merged
merged 12 commits into from
Feb 16, 2022
Merged

refactor: convert CLI entry points to ESM; migrate create-docusaurus to ESM #6661

merged 12 commits into from
Feb 16, 2022

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented Feb 12, 2022 •
edited
Loading

Motivation

#6520

This is a safe move, because CLI entry points aren't imported in any file.

For now, we are using createRequire(import.meta.url) to import the package.json files, but we may explore using JSON modules in the future.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Tests

@Josh-Cena Josh-Cena added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Feb 12, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 12, 2022
@Josh-Cena Josh-Cena mentioned this pull request Feb 12, 2022
Open
1 task
@netlify
Copy link

netlify bot commented Feb 12, 2022 •
edited
Loading

✔️ [V2]

🔨 Explore the source changes: 307da2c

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/620d0df05fa7a70008c352b6

😎 Browse the preview: https://deploy-preview-6661--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Feb 12, 2022 •
edited
Loading

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 52
🟢 Accessibility 100
🟢 Best practices 100
🟢 SEO 100
🟠 PWA 83

Lighthouse ran on https://deploy-preview-6661--docusaurus-2.netlify.app/

@Josh-Cena Josh-Cena changed the title refactor: convert CLI entry points to ESM refactor: convert CLI entry points to ESM; migrate create-docusaurus to ESM Feb 12, 2022
@github-actions
Copy link

github-actions bot commented Feb 12, 2022 •
edited
Loading

Size Change: +1.24 kB (0%)

Total Size: 780 kB

Filename Size Change
website/build/assets/css/styles.********.css 105 kB +54 B (0%)
website/build/assets/js/main.********.js 589 kB +663 B (0%)
website/build/index.html 37.6 kB +526 B (+1%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 48 kB

compressed-size-action

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Feb 12, 2022 •
edited
Loading

@slorber Now here's a good question worth considering as we upgrade. Do you have strong opinions about using import _ from 'lodash'? Because using named imports from lodash is problematic (as I told you previously, not all named exports can be statically determined), you either have to use default imports, or swap lodash with lodash-es. I actually think import _ from 'lodash' is not a bad idea, but maybe lodash-es can have some performance benefits (because it doesn't execute the entire module)? The lodash import is cached anyways, so I think there isn't a need to use lodash-es.

slorber
slorber approved these changes Feb 16, 2022
View reviewed changes
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

LGTM 👍

packages/docusaurus/src/index.ts
export {default as clear} from './commands/clear';
export {default as writeTranslations} from './commands/writeTranslations';
export {default as writeHeadingIds} from './commands/writeHeadingIds';
import build from './commands/build';
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this really needed? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, because of some weird TS transpilation output... It's not as sane as exports.clear = require('./commands/clear').default. Doesn't look much worse this way

@slorber
Copy link
Collaborator

slorber commented Feb 16, 2022

no strong opinions on lodash to block this PR

As long as it works and it's only in the nodejs side I don't think the impact is huge, but we should probably migrate to lodash-es someday, not necessarily today

@Josh-Cena
Copy link
Collaborator Author

I actually have thought about using _.sortBy and such for a long time. Looks much more understandable to me than a lone sortBy

@Josh-Cena Josh-Cena merged commit 67918e3 into main Feb 16, 2022
@Josh-Cena Josh-Cena deleted the jc/entry-esm branch February 16, 2022 15:00
@Josh-Cena Josh-Cena mentioned this pull request Feb 19, 2022
Merged
@mikeharder mikeharder mentioned this pull request Sep 29, 2023
Merged
mikeharder added a commit to microsoft/typespec that referenced this pull request Sep 29, 2023
@mikeharder
Force upgrade dep update-notifier from 5.1.0 to 6.0.2 (#2516)
393f907 
- update-notifier is only used by @docusaurus/core
- update-notifier@6 is pure ESM (a breaking change)
- docusaurus entry points are ESM (facebook/docusaurus#6661), so the
package is still compatible
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slorber slorber slorber approved these changes

@lex111 lex111 Awaiting requested review from lex111

Assignees
No one assigned
Labels
CLA Signed Signed Facebook CLA pr: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Josh-Cena @slorber @facebook-github-bot