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: configure proper pages directory for next application #18009

Merged
merged 3 commits into from
Sep 14, 2021

Conversation

ZachJW34
Copy link
Contributor

@ZachJW34 ZachJW34 commented Sep 7, 2021

User facing changelog

NA

Additional details

Our Next.js plugin was not pointing to the correct pagesDir, causing errors to be thrown if files under projectRoot did not follow the pages export convention. This PR sets the pagesDir by looking for either {root}/pages or {root}/src/pages (Next.js conventions) and falling back to {root} if not found.

I tested this by checking out https://github.com/cypress-io/cypress-component-testing-examples and:

  1. cd ./create-next-app
  2. yarn
  3. mkdir -p ./src/components
  4. echo "export const HelloWorld = () => 'Hello World!'" >> ./src/components/HelloWorld.js
  5. echo "export * from './HelloWorld'" >> ./src/components/index.js
  6. Add import { HelloWorld } from '../src/components' to ./pages/index.js
  7. npx cypress run-ct and see that it fails (You can run DEBUG=* npx cypress run-ct to see logs)
  8. Copy new plugins folder from this branch (or entire built package) into ./node_modules/@cypress/react
  9. npx cypress run-ct and see that it passes

How has the user experience changed?

NA

PR Tasks

  • Have tests been added/updated?
  • Has the original issue or this PR been tagged with a release in ZenHub?
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?
  • Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Sep 7, 2021

Thanks for taking the time to open a PR!

@ZachJW34 ZachJW34 changed the base branch from develop to master September 7, 2021 17:41
@cypress
Copy link

cypress bot commented Sep 7, 2021



Test summary

4181 0 53 1Flakiness 4


Run details

Project cypress
Status Passed
Commit 9445ee4
Started Sep 13, 2021 4:05 AM
Ended Sep 13, 2021 4:16 AM
Duration 10:52 💡
OS Linux Debian - 10.9
Browser Firefox 89

View run in Cypress Dashboard ➡️


Flakiness

commands/xhr_spec.js Flakiness
1 ... > no status when request isnt forced 404
2 src/cy/commands/xhr > abort > aborts xhrs currently in flight
commands/net_stubbing_spec.ts Flakiness
1 network stubbing > intercepting request > can intercept utf-8 request bodies without crashing
cypress/proxy-logging-spec.ts Flakiness
1 Proxy Logging > request logging > fetch log shows resource type, url, method, and status code and has expected snapshots and consoleProps

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Copy link
Contributor

@JessicaSachs JessicaSachs left a comment

Choose a reason for hiding this comment

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

This looks good, but we need to make sure that our code is written ts-first. If you have a sec to convert the new Javascript files into Typescript, that would be super.

@@ -12,6 +12,9 @@
"cy:run:debug": "node --inspect-brk ../../scripts/start.js --component-testing --run-project ${PWD}",
"pretest": "yarn transpile",
"test": "yarn cy:run",
"yarn": "yarn test-unit && yarn test-component",
"test-unit": "mocha test/**/*.spec.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"test-unit": "mocha test/**/*.spec.js",
"test-unit": "mocha test/**/*.spec.*",

return projectRoot
}

module.exports.findPagesDir = findPagesDir
Copy link
Contributor

Choose a reason for hiding this comment

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

For example, in TS this would be

export function findPagesDir() { /* ... */ }

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Nice job figuring this out - just need some TS and to clarify what the yarn command is for (yarn does not seem like a great name for a script in package.json).

@@ -12,6 +12,9 @@
"cy:run:debug": "node --inspect-brk ../../scripts/start.js --component-testing --run-project ${PWD}",
"pretest": "yarn transpile",
"test": "yarn cy:run",
"yarn": "yarn test-unit && yarn test-component",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this step supposed to be called yarn? This doesn't seem correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I meant to replace line 14 with "test": "yarn test-unit && yarn test-component",

@ZachJW34
Copy link
Contributor Author

ZachJW34 commented Sep 9, 2021

@lmiller1990 @JessicaSachs Is the appveyor CI failure something I should be concerned about? I can't reproduce the error locally. If I do need to make a change to how I'm compiling the typescript I see two ways:

  1. Move the findPagesDir.ts into the src/ directory. This makes sense because the rootDir is set as src so that's where TS files should be located.
  2. Create a separate tsconfig inside of plugins.

There are more drastic changes we could make but I didn't want to change too much just for a small bug fix. Thoughts?

JessicaSachs
JessicaSachs previously approved these changes Sep 10, 2021
Copy link
Contributor

@JessicaSachs JessicaSachs left a comment

Choose a reason for hiding this comment

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

LGTM. No, Appveyor sometimes doesn't retrigger. It's usually fine, though.

@lmiller1990
Copy link
Contributor

Seems the build is broken. You should be able to reproduce locally with yarn build @ZachJW34

@ZachJW34
Copy link
Contributor Author

@lmiller1990 Looks like windows didn't like the glob pattern for tsc cli. I added a tsconfig for plugins so it should be good now.

@lmiller1990 lmiller1990 self-requested a review September 14, 2021 03:17
@lmiller1990 lmiller1990 merged commit 70c7c36 into master Sep 14, 2021
@ZachJW34 ZachJW34 mentioned this pull request Sep 23, 2021
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.

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