Skip to content

Conversation

@pieh
Copy link
Contributor

@pieh pieh commented Sep 25, 2020

This PR bundles quite a bit of dependencies which cause problems for users (react, @mdx-js/*) when there are multiple version of those installed in node_modules hierarchy is not quite right

Closes #27278


This is part of PR series:

  1. feat(gatsby-dev-cli): install deps if there are no gatsby deps but --forceInstall was used
  2. test(integration/gatsby-cli): use sandboxed directory to "globally" install gatsby-cli
  3. chore(gatsby-recipes): bundle dependencies (THIS PR)
  4. chore(gatsby-cli): bundle dependencies

[ch17886]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 25, 2020
@pieh pieh removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 25, 2020
@pieh pieh force-pushed the chore/bundle-cli-and-recipes/sandbox-cli-tests branch from e197fee to 9c48758 Compare September 30, 2020 09:52
@pieh pieh force-pushed the chore/bundle-cli-and-recipes/gatsby-recipes branch from 4bff535 to df712b4 Compare September 30, 2020 09:52
@pieh pieh force-pushed the chore/bundle-cli-and-recipes/gatsby-recipes branch from df712b4 to 799f2f9 Compare October 4, 2020 20:04
@pieh pieh changed the base branch from chore/bundle-cli-and-recipes/sandbox-cli-tests to master October 4, 2020 20:06
@gatsby-cloud
Copy link

gatsby-cloud bot commented Oct 4, 2020

Gatsby Cloud Build Report

gatsby

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

Build Details

View the build logs here.

🕐 Build time: 21m

@pieh pieh force-pushed the chore/bundle-cli-and-recipes/gatsby-recipes branch 2 times, most recently from dfe1502 to 778e214 Compare October 5, 2020 08:50
Comment on lines 52 to 54
"remark-mdx": "^2.0.0-next.4",
"remark-mdxjs": "^2.0.0-next.4",
"remark-parse": "^6.0.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are additional candidates for bundling - I wasn't sure if those cause problems or not, so for now those are not bundled

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.

some small nits

NOICE

@pieh pieh force-pushed the chore/bundle-cli-and-recipes/gatsby-recipes branch 2 times, most recently from 95edecb to 0d79ce8 Compare October 23, 2020 11:01
@pieh pieh force-pushed the chore/bundle-cli-and-recipes/gatsby-recipes branch from 0d79ce8 to 3c2effc Compare October 26, 2020 11:45
@pieh pieh requested review from a team October 26, 2020 11:45
@pieh pieh merged commit 77143e8 into master Oct 26, 2020
@delete-merged-branch delete-merged-branch bot deleted the chore/bundle-cli-and-recipes/gatsby-recipes branch October 26, 2020 13:46
"terminal-link": "^2.0.0",
"tmp-promise": "^2.1.0"
"tmp-promise": "^2.1.0",
"urql": "^1.9.7"
Copy link

@kitten kitten Nov 12, 2020

Choose a reason for hiding this comment

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

Hiya 👋 I'm just wondering; I did notice that this dependency on urql is getting a little old, so I'm wondering whether anyone on the Recipes team has tried out the newer versions? Also, I'm a little confused about the aim of this PR. I see the problems with MDX above, but a lot of dependencies seem to now selectively either bundled or not bundled at random ✌️ wouldn't it be consequent then to consequently bundle all dependencies that become part of the CLI?

Copy link
Contributor Author

@pieh pieh Nov 12, 2020

Choose a reason for hiding this comment

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

I think I put it in there because of urql's peerDependency on react - if I didn't, then it would resolve to different react version that other things in the recipes cli use, so that caused problems. Please also note that this is CLI react (ink) and not web target to make things more confusing.

Essentially I bundled everything that had any connection to react here as that was causing problems with react version provided by user (but as mention in description - this is special case for the way npm install dependencies, we really shouldn't need to do it (and I rather not do this and let package manager properly resolve node_modules hierarchy, but that's not the reality unfortunately). So this is workaround on npm quirk, rather than problem with the way we, or deps we bundled are setup).

Few of "tickets" that are about similar issues with npm:

Even if those got fixed in recent npm version (didn't try), unfortunately our users will also use older versions (like those autoinstalled with Node itself)

As for bundling everything: I did want to limit the scope of bundling to those packages that cause the problems for users (again due to npm quirk, and NOT because packages themselves are problematic). Those that didn't cause (known) problem I did leave as they were - there is some value in not bundling more than needed in node, because there is less duplication as shared packages can be parsed/executed just once (i.e. I explicitly mentioned potential candidates for bundling that I left out not bundled here in #27057 (comment) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did notice that this dependency on urql is getting a little old

From what I can latest of urql is 1.11.2, so this should be covered by that version range - but granted, because we do bundle urql now, we will use version that we have specified in lock file

Copy link

Choose a reason for hiding this comment

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

That makes sense! The first impression I got was that some packages seemed related to the UI but aren't bundled, which seems a little odd at first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[gatsby-plugin-mdx] + [gatsby-recipes]: Mixed mdx versions (2.x alpha being pulled in)

4 participants