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

chore(gatsby-cli): bundle ink logger #26887

Merged
merged 5 commits into from Sep 15, 2020
Merged

chore(gatsby-cli): bundle ink logger #26887

merged 5 commits into from Sep 15, 2020

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Sep 14, 2020

Description

This PR looks to bundle parts of gatsby-cli which use ink and react. In some scenarios multiple versions of react can be installed in node_modules and this can lead to very weird errors and warnings related to multiple react instances. This change work around it, so react is being bundled and is part of published npm package instead as dependency that users need to download.

For more information check on description of #21522 which highlights some problems with multiple versions of react (it seems that peerDependency is not really honored by npm when resolving node_modules hierarchy and this is why it cause such a problems).

Related Issues

Related to #19827, #21522

@pieh pieh added the type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change label Sep 14, 2020
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 14, 2020
@pieh pieh removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 14, 2020
@gatsby-cloud
Copy link

gatsby-cloud bot commented Sep 14, 2020

Gatsby Cloud Build Report

gatsby

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 25m

This is needed for PnP as setting `yoga-layout-prebuilt` as external
imports it from node_modules, but it's not declared by gatsby-cli itself.

Alternatively we could bundle it too, but this has problems too:
facebook/yoga#798
the solution there is pretty hacky and I would rather not use it.
Comment on lines +63 to +67
// Next one deserve explanation: ... it's because ink logger imports
// getStore, onLogAction from higher up (../../redux). But we don't want
// two copies of it - one bundled and one not, because it would result
// in multiple store copies
`../../redux`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding inline comment here to highlight this - this is the most fragile part of this setup

wardpeet
wardpeet previously approved these changes Sep 14, 2020
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Tested and it fixes the npm issue when react is a lower version than 16.8.0.

packages/gatsby-cli/.babelrc.json Outdated Show resolved Hide resolved
wardpeet
wardpeet previously approved these changes Sep 14, 2020
@pieh pieh merged commit fe596ec into master Sep 15, 2020
@delete-merged-branch delete-merged-branch bot deleted the chore/bundle-cli branch September 15, 2020 09:45
@pieh
Copy link
Contributor Author

pieh commented Sep 16, 2020

Published in gatsby@2.24.61 / gatsby-cli@2.12.98

pieh added a commit that referenced this pull request Sep 16, 2020
gatsbybot pushed a commit that referenced this pull request Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants