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: require babel dependencies directly #1575

Merged
merged 1 commit into from
Jun 14, 2019
Merged

Conversation

Elrendio
Copy link
Contributor

@Elrendio Elrendio commented Jun 6, 2019

Motivation

FIX #1574

Have you read the Contributing Guidelines on pull requests? YES

Test Plan

  1. Follow the reproducing steps of Docusaurus does not require *its* dependencies via babel when building (crashes when installed via pnpm) #1574
  2. Check that docusaurus builds

Related PRs

This is a fix, it shouldn't require any updates in the documentation.

@Elrendio Elrendio requested a review from endiliey as a code owner June 6, 2019 15:22
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jun 6, 2019

Deploy preview for docusaurus-2 ready!

Built with commit 54c9e1b

https://deploy-preview-1575--docusaurus-2.netlify.com

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jun 6, 2019

Deploy preview for docusaurus-preview ready!

Built with commit 54c9e1b

https://deploy-preview-1575--docusaurus-preview.netlify.com

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Some changes that need to be made before we can proceed:

  • Since you've read the contributing guidelines - please follow the Semantic Commit Messages
  • The CI checks are failing
  • The PR need a clearer title that explains the PR - this will eventually become the commit message if/when merged

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 6, 2019
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@Elrendio Elrendio changed the title FIX - facebook/Docusaurus#1574 fix: require babel dependencies directly Jun 6, 2019
**What it does:**
 - Directly require babel dependencies instead of asking babel to do it.
@Elrendio
Copy link
Contributor Author

Elrendio commented Jun 6, 2019

Thanks for the PR! Some changes that need to be made before we can proceed:

  • Since you've read the contributing guidelines - please follow the Semantic Commit Messages
  • The CI checks are failing
  • The PR need a clearer title that explains the PR - this will eventually become the commit message if/when merged

Hello yangshun,

Thanks for the feedbacks, I've updated the PR title, the commit message and linted the files.

@Elrendio Elrendio changed the title fix: require babel dependencies directly fix(v1): require babel dependencies directly Jun 6, 2019
Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Looks good to me. cc @Ten0 @endiliey does this look ok to you?

@yangshun yangshun changed the title fix(v1): require babel dependencies directly fix: require babel dependencies directly Jun 6, 2019
@@ -13,10 +13,13 @@ require('@babel/register')({
only: [__dirname, `${process.cwd()}/core`],
plugins: [
require('./server/translate-plugin.js'),
'@babel/plugin-proposal-class-properties',
'@babel/plugin-proposal-object-rest-spread',
Copy link
Contributor

Choose a reason for hiding this comment

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

is the default really needed ?

Copy link

Choose a reason for hiding this comment

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

It seems to crash otherwise, something like "default is not a valid property for a plugin".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes because the modules are coded to be used with es6 imports syntax when babel requires commonJS imports.

@endiliey endiliey merged commit a599682 into facebook:master Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docusaurus does not require *its* dependencies via babel when building (crashes when installed via pnpm)
6 participants