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

Component testing + Next JS using @cypress/react/plugins/next configures the 'pages' directory wrongly #17898

Closed
rafaelchiti opened this issue Aug 26, 2021 · 18 comments · Fixed by #18009

Comments

@rafaelchiti
Copy link

Current behavior

I believe that the internal configuration from the @cypress/react/plugins/next configures 'nextjs' to think that the pages directory is the 'root folder of the project' rather than ${rootFolder}/pages.

If you look at this line:
https://github.com/cypress-io/cypress/blob/develop/npm/react/plugins/next/findNextWebpackConfig.js#L29
Screen Shot 2021-08-26 at 4 06 35 PM

The findNextWebpackConfig util is setting the pagesDir to the root of the project.

Consequences

Because of that I'm seeing that next js webpack config is treating every folder in my root as a 'pages' folder, trying to bundle them as if they were pages. I bumped into this because I started seeing the following error while running component tests:

Syntax error: Using `export * from '...'` in a page is disallowed. Please use `export { default } from '...'` instead.
Read more: https://nextjs.org/docs/messages/export-all-in-page

At first I thought I had some 'export * from ...' inside a 'page' but that was not the case. I found that a file inside ${root}/src/UILibrary/icons was doing export * from .... And because of how the cypress next plugin is configuring webpack my /src/UILibrary/icons is being treated as a page by next+webpack.

Desired behavior

Not fully certain of the solution. Probably allow to pass that configuration instead of assuming that pagesDir is config.projectRoot. So we can do the following inside our plugins config file:

const injectDevServer = require("@cypress/react/plugins/next");

module.exports = (on, config) => {
  if (config.testingType === "component") {
    injectDevServer(on, {...config, pagesDir: 'path to my pages dir' });
  }

  return config;
};

Optionally we could have a better default pointing to ${root}/pages (if it exists) or to ${root}/src/pages (which is the second option that nextjs recognizes as convention.

Test code to reproduce

I forked the webpack example from cypress docs and added a export * from ... to trigger the error. (this happens in webpack 4 or webpack 5 config the same. I used the webpack 4 because I wanted to use the official example published by cypress with the minimum amount of changes).

Here the fork and branch where to try it: https://github.com/rafaelchiti/cypress-component-examples/tree/bug_repro

To see the error make sure you are on that branch bug_repo and run:
cd nextjs-webpack-4 to focus on the next example
yarn install --pure-lockfile
DEBUG=* yarn run cypress run-ct
If you don't set a wide debug filter option then the error gets swollen (probably there is a more limited debug config that still prints the error). Then after running that you should find in the terminal an error like:

Syntax error: Using `export * from '...'` in a page is disallowed. Please use `export { default } from '...'` instead.
Read more: https://nextjs.org/docs/messages/export-all-in-page

But if you look in the code there is no export * from ... inside the pages directory. But there is though an export * from ... inside the src/icons directory (which I added to test the problem).

Cypress Version

7.2.0 also tried on 8.3.0

Other

No response

@AndreSilva1993
Copy link

AndreSilva1993 commented Aug 26, 2021

Was about to create an issue for this as well. I think one of the solutions, like @rafaelchiti mentioned would be to let us pass that configuration to the @cypress/react/plugins/next package and if no value was passed, one could assume the existing config.projectRoot. Getting the exact same error as Next.js is interpreting every single component I import as a page.

@cassus
Copy link

cassus commented Aug 26, 2021

Wow, I've been tracking down the same bug just now and created a
minimal project to demonstrate the issue with webpack 5: https://github.com/cassus/cypress-nextjs-export-star-error-example

Thanks for all the investigation you put into this @rafaelchiti

@cassus
Copy link

cassus commented Aug 26, 2021

I can also confirm that I could avoid the issue by replacing pagesDir: config.projectRoot, with pagesDir: `${config.projectRoot}/pages`, at https://github.com/cypress-io/cypress/blob/develop/npm/react/plugins/next/findNextWebpackConfig.js#L29

@JessicaSachs
Copy link
Contributor

@cassus thank you for the workaround. We'll triage this. @ZachJW34 is this something you could look at today?

@ZachJW34
Copy link
Contributor

Yes, I can take a look later today!

@ZachJW34
Copy link
Contributor

@rafaelchiti Thanks for all the detail, it's was super helpful. I tested out your repo and confirmed the issue (and the fix by changing pagesDir). I think the best solution is to do what Next does and look for a ${root}/pages or a ${root}/src/pages directory and use that value rather than adding a configuration option (which they seem to be against). I can get a PR for this early next week.

@rafaelchiti
Copy link
Author

@rafaelchiti Thanks for all the detail, it's was super helpful. I tested out your repo and confirmed the issue (and the fix by changing pagesDir). I think the best solution is to do what Next does and look for a ${root}/pages or a ${root}/src/pages directory and use that value rather than adding a configuration option (which they seem to be against). I can get a PR for this early next week.

That sounds great, agree about making it a convention, wouldn't make sense to make it configurable when next js itself doesn't allow configuration for it. Thanks 🙌.

@AndreSilva1993
Copy link

Any news on this one?

@rafaelchiti
Copy link
Author

Any news on this one?

(I'm not speaking on behalf of the team but just in case this is useful) While the PR gets merged and the release cut, is straightforward to copy paste the files used and modify manually, then you can remove them once the new release is out. You only need to cc the following files (and adjust the import paths):

And then in your plugins file you import the 'injectDevServer' from your cc'ed version (the ...plugins/next/index.js file).
And modify the file findNextWebpackConfig.js so it points to the right pages folder:

pagesDir: `${config.projectRoot}/pages`,

I know is not ideal but will get you going until this gets released.

@AndreSilva1993
Copy link

Any news on this one?

(I'm not speaking on behalf of the team but just in case this is useful) While the PR gets merged and the release cut, is straightforward to copy paste the files used and modify manually, then you can remove them once the new release is out. You only need to cc the following files (and adjust the import paths):

And then in your plugins file you import the 'injectDevServer' from your cc'ed version (the ...plugins/next/index.js file).
And modify the file findNextWebpackConfig.js so it points to the right pages folder:

pagesDir: `${config.projectRoot}/pages`,

I know is not ideal but will get you going until this gets released.

That’s what I’ve been using for development purposes but my pipelines break, hence my question 👌

@cypress-bot cypress-bot bot added stage: work in progress stage: needs review The PR code is done & tested, needs review and removed stage: work in progress stage: needs review The PR code is done & tested, needs review labels Sep 7, 2021
@lmiller1990
Copy link
Contributor

This PR will fix the problem and be automatically released via semantic release once it is merged into master.

@cypress-bot cypress-bot bot added stage: needs review The PR code is done & tested, needs review and removed stage: work in progress labels Sep 10, 2021
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Sep 14, 2021

The code for this is done in cypress-io/cypress#18009, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

@cypress-bot cypress-bot bot added stage: pending release and removed stage: needs review The PR code is done & tested, needs review labels Sep 14, 2021
@lmiller1990
Copy link
Contributor

lmiller1990 commented Sep 15, 2021

This should be fixed now. The latest version of @cypress/webpack-dev-server was released 5 days ago when #18009 was merged up.

If this is still a problem, we can reopen and look into it again. Thanks!

@rafaelchiti
Copy link
Author

The fix for this is inside @cypress/react any idea of when that is going to be released? since the latest @cypress/react version (5.10.0) does not include the fix. Thanks 🙏

@lmiller1990
Copy link
Contributor

lmiller1990 commented Sep 20, 2021

We use semantic release so anything merged into master should be released immediately. It looks like this didn't happen - maybe something failed in CI, so the release didn't go ahead.

I will try re-running the master pipeline, which should trigger a release.

The standard release cycle for Cypress is every two weeks, so worst case, it'll be out in two weeks. In this case, I'd like this to be released immediately, so I'll look into it.

Edit: it looks like the semantic release step is failing. I think we rotated some tokens but this got missed. I'll raise this internally.

@AndreSilva1993
Copy link

Was this released already? It’s not included in the 8.5.0 release notes

@lmiller1990
Copy link
Contributor

lmiller1990 commented Sep 29, 2021

Seems semantic release is still problematic, so any changes under npm in the last 8 days have not been released. Looking into this. It should be out this week.

The 8.5.0 release notes are only for the main Cypress app, everything under npm/* in this repo is released separately via semantic release (when merged into master).

@ZachJW34
Copy link
Contributor

ZachJW34 commented Oct 6, 2021

@AndreSilva1993 @rafaelchiti this change has been released in v5.10.1

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 a pull request may close this issue.

8 participants