[Feature] Improve the Gatsby E2E

[kevin940726]

• I'd be willing to implement a fix (if I actually found a root cause 😂)

Describe the bug

As mentioned in #681 (comment). As from the latest version of gatsby, yarn berry no longer work properly with it. yarn build works perfectly, but yarn start will result in an error shown below.

 ERROR #98123  WEBPACK

Generating development JavaScript bundle failed

Failed to load config "react-app" to extend from.
Referenced from: BaseConfig

File: .cache/app.js

failed Building development bundle - 0.460s

To Reproduce

Update the gatsby dependency to the latest version by running

yarn workspace @yarnpkg/gatsby add gatsby

Then run the development server

yarn workspace @yarnpkg/gatsby start

We can also reproduce it by creating a new gatsby project

yarn dlx gatsby new berry-gatsby
cd berry-gatsby
yarn start

We only covered yarn build in our e2e test, so it didn't catch that.

Sherlock:

const path = require('path');

await yarn('dlx', 'gatsby', 'new', 'berry-gatsby');
process.chdir(path.resolve(process.cwd(), 'berry-gatsby'));
await yarn('build');
await yarn('dlx', 'start-server-and-test', '"yarn start"', ':8000', '"curl -s localhost:8000 | grep \"<h1>Hi people</h1>\""');

Screenshots

Environment if relevant (please complete the following information):

• OS: macOS 10.15.2
• Node version: 10.15.3
• Yarn version: 2.0.0-rc.21

Additional context

Note that it's not happening with the current version we used in our website (2.13.6). Looks like the problem is with eslint-loader or more specifically how gatsby uses it. By filtering out the eslint-loader rule in webpack config can no longer reproduce the issue. I haven't looked into the source or diff the versions to find the root cause yet. I'll continue try to find it, it's fun!

Update 1:

I'm able to pin point the problem to eslint-config-react-app. Downgrading it to ^4 fixes the issue. I'll continue to try to find what goes wrong.

Update 2:

Seems like it's happening because of eslint actually. Especially in this line when it tries to call Module.createRequire('CWD/__placeholder__.js').require('eslint-config-react-app') will results in a failure. I'll try to create a minimal reproduce steps for eslint.

Update 3:

Since that gatsby bundles eslint-config-react-app inside the gatsby package, so that the user won't have to install that package themselves. However, requiring the package is handled by eslint, which requires from cwd, i.e. the user package root, hence the error.
I have no idea how to fix it now though, I'm sure that it's something happens a lot in other packages too, maybe @arcanis has an idea?

[arcanis]

Hey @kevin940726 ! Thanks a lot for investigating ! I have a few comments:

First, I remember there were a few similar issues in the past. I fixed that by adding "fallbacks", where if a package is missing then we still try to see whether it's a dependency of Gatsby (and react-scripts, for CRA): https://github.com/yarnpkg/berry/blob/master/packages/yarnpkg-pnp/sources/loader/makeApi.ts#L52-L64. That was a long time ago, almost a year.

Then a few months ago, I implemented pnpFallbackMode which restricts which packages are allowed to use the fallback. By default, workspaces can't do it. I think that's when the Gatsby compatibility broke (but since we only run yarn build in the E2E, ESLint never ran and I didn't see the problem until you raised it).

Now as how to fix, it: I didn't think it would still be a problem actually! From my understanding, ESLint 6 should load configs and plugins relative to the configuration that imports them, so I'm surprised this isn't the case - do you have an idea why?

More precisely, ESLint v6 resolves plugins relative to the end user's project by default, and always resolves shareable configs and parsers relative to the location of the config file that imports them.

[arcanis]

Found the problem! I've opened gatsbyjs/gatsby#20629 which I think should fix that - can you confirm @kevin940726, to make sure I'm not missing something?

[kevin940726]

@arcanis Just tried that and it works perfectly!

I can help add e2e tests to the gatsby workflow for this, and we can merge it after the upstream released a new version.

I believe there are still some other issues with popular gatsby plugins though. I'll keep investigating.

One question: why do we want to provide require.resolve to eslint-config-react-app, but not for other plugins like graphql? Is this related to how eslint treats extends differently than plugins?

[arcanis]

Yep, I think Eslint allows config paths to be absolute because it doesn't matter if the same config is loaded twice, but they don't support absolute plugin paths because reason.

Imo the "plugin relative path" option should also be applied to the config lookup, but since it isn't.. Easier to use an absolute path here than open an issue on Eslint, especially given a change here would likely be breaking 😛

I can help add e2e tests to the gatsby workflow for this, and we can merge it after the upstream released a new version.

Awesome! I think we can even start figuring out what the develop test should look like without waiting for upstream to release the fix; we'll just hold off on the merge until the release.

I think the easiest might be to spawn the process and aggregate its output somewhere, wait until it publishes the initial bundle to the local port, then look for errors in the buffered logs. Wdyt?

[kevin940726]

Some updates on the plugins I found which are incompatible to berry.

gatsby-plugin-mdx uses remark-parse, remark-squeeze-paragraphs, @babel/plugin-syntax-jsx, and @babel/plugin-syntax-object-rest-spread in their loader. But these dependencies are not listed by the plugin itself, but by a peer dependency @mdx-js/mdx. I think it breaks the resolution process of berry?

I have 2 solutions for now.

1. List the dependencies in gatsby-plugin-mdx.
2. Use Module.createRequire or Module.createRequireFromPath to load these dependencies on behave of @mdx-js/mdx.

Personally I think (2) would be a better choice as the maintainer don't have to remember to also update all the dependencies when updating @mdx-js/mdx. And it better fits the original purpose of the code?

I wonder what's the general solution for this kind of problem. I've already implemented (2) locally (and tested) and ready to send the PR to gatsby if that sounds good to you.

One addition problem would be that gatsby supports node version >= 8, while createRequireFromPath only added in v10.12.0. Currently I just fallback it to the regular require in this case, but it would still break in berry running node version under v10.12.0. Is that something we should cover too?

As long as it's done, I can add gatsby-plugin-mdx to the e2e tests too, since it's a popular (and official) plugin of gatsby.

[kevin940726]

I think the easiest might be to spawn the process and aggregate its output somewhere, wait until it publishes the initial bundle to the local port, then look for errors in the buffered logs. Wdyt?

That's exactly what I thought! We can use start-server-and-test to listen to the port and assert the output. We still have to find a way to filter the error logs though. I think we have to follow some conventions here, though not guaranteed to be stable across versions (gatsby could change the error log to something else completely different), but at least it makes sense for us to update the tests when it happened.

[arcanis]

Personally I think (2) would be a better choice as the maintainer don't have to remember to also update all the dependencies when updating @mdx-js/mdx. And it better fits the original purpose of the code?

I'd usually recommend (1) as it makes the dependency tree more explicit. That said, createRequire is still a new API and maybe a new pattern may emerge that would make sense for this kind of things; so why not!

One addition problem would be that gatsby supports node version >= 8, while createRequireFromPath only added in v10.12.0. Currently I just fallback it to the regular require in this case, but it would still break in berry running node version under v10.12.0. Is that something we should cover too?

Gatsby already has a util for this here. If we go down the createRequire path we can reuse it, or port it.

start-server-and-test

Nice! Wasn't aware of this tool, it seems to be what we'd be looking for 👍 As for the logs, maybe we can just grep for ERROR # ?

[kevin940726]

Gatsby already has a util for this here.

Oops! I didn't notice that they have this! This is nice, looks like we can just use this 👍

I guess that I can open up a PR to gatsby with the second approach, and briefly mention the alternative solution (1) with this issue to them, and let them decide which one to adopt 😉.

maybe we can just grep for ERROR #?

~Yah that makes the most sense, and it's all we can do for now. Let's just hope that webpack/gatsby won't change their error output anytime soon 😂

I wonder if there're some kind of secret options for webpack to output the log differently when in CI environment 🤔. I'll do some digging.~

Hmmm, nvm... I think for now all we have to do is to check that localhost:8000 is reachable. A simple GET with status code 200 should do the trick?

[arcanis]

The fix has been released!

Note to self: I need to check Sherlock, something changed and it can't send comments anymore:
https://github.com/yarnpkg/berry/commit/845b320bd1472fbcc2b03fb000ccf35cd7c84507/checks?check_suite_id=405634039#step:7:40

[kevin940726]

It has now been merged 🎉! I think we can close this now. Let's add following issues if there are any.