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.

Sign up for GitHub

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

Jump to bottom

fix(cypress/webpack-dev-server): correct path for indexHtmlFile #27320

Merged
merged 3 commits into from
Aug 4, 2023
Merged

fix(cypress/webpack-dev-server): correct path for indexHtmlFile #27320

merged 3 commits into from
Aug 4, 2023

Conversation

lmiller1990
Copy link
Contributor

Additional details

We are not passing the correct path to the indexHtmlFile. If cypress.config.js is not in the root of the project, the path is wrong and webpack errors.

Steps to test

I added a test. Alternatively test against my minimal reproduction https://github.com/lmiller1990/cypress-test-tiny/compare/master...lmiller1990:cypress-test-tiny:issue-26400?expand=1

How has the user experience changed?

Can have cypress.config in a directory that is not the root of the project.

PR Tasks

@lmiller1990 lmiller1990 mentioned this pull request Jul 18, 2023
Closed
lmiller1990
lmiller1990 commented Jul 18, 2023
View reviewed changes
npm/webpack-dev-server/src/makeDefaultWebpackConfig.ts Outdated
@@ -78,7 +78,7 @@ export function makeCypressWebpackConfig (
},
plugins: [
new (HtmlWebpackPlugin as typeof import('html-webpack-plugin-5'))({
template: indexHtmlFile,
template: path.join(projectRoot, indexHtmlFile),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some tests are failing on this line: https://app.circleci.com/pipelines/github/cypress-io/cypress/54632/workflows/e04d8993-cb44-42a9-9061-e7b37bef32f2/jobs/2257912

Looks like they are missing setting the indexHtmlFile value

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I fixed this in b557e6e. If indexHtmlFile is undefined, we'll just pass undefined like we've been doing. Otherwise we'll call join to create the path

lmiller1990
lmiller1990 commented Jul 18, 2023
View reviewed changes
system-tests/project-fixtures/react/webpack.config.js Outdated
@@ -18,6 +18,11 @@ module.exports = {
exclude: /node_modules/,
use: {
loader: 'babel-loader',
options: {
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 had to add these, I'm not sure why it was not picking up the .babelrc. Either way, it's unrelated to this bug fix - although there might be another, unrelated issue around babel-loader not automatically finding .babelrc (that was not introduced by this PR).

If that is an issue, can look into fixing it separately - no bandwidth to go deeper into specifics right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this working before the changes in this PR? How did you know that babel was not loading correctly? Did you see an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is complaining about not knowing about JSX - I think what's happening is some other part of the toolchain (webpack) expects .babelrc to exist in the same directory where the process is running, which is now a nested directory (in cypress/ in this case.

I am not sure if this going to impact users. It's likely this is a problem no-one has encountered, since you can't current run with a nested config file, anyway.

Either way, I will create a more minimal project are requested that will side step this issue. Projects using babel and relying on .babelrc might run into this, but it'll be a separate and unrelated bug...

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 pushed a simple project that won't deal with this. The fact webpack relies on the cwd to find the babelrc might be an issue but I'm not sure that's even our problem to solve.

@lmiller1990 lmiller1990 requested a review from a team July 18, 2023 04:05
@cypress
Copy link

cypress bot commented Jul 18, 2023 •
edited
Loading

Passing run #49411 ↗︎

0 635 0 0 Flakiness 0

Details:

Merge branch 'develop' into lmiller/issue-26400
Project: cypress Commit: edb27585dc
Status: Passed Duration: 07:05 💡
Started: Aug 4, 2023 3:45 AM Ended: Aug 4, 2023 3:52 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

warrensplayer
warrensplayer reviewed Jul 18, 2023
View reviewed changes
npm/webpack-dev-server/src/makeDefaultWebpackConfig.ts Outdated
@@ -78,7 +78,7 @@ export function makeCypressWebpackConfig (
},
plugins: [
new (HtmlWebpackPlugin as typeof import('html-webpack-plugin-5'))({
template: indexHtmlFile,
template: path.join(projectRoot, indexHtmlFile),
Copy link
Contributor

Choose a reason for hiding this comment

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

Some tests are failing on this line: https://app.circleci.com/pipelines/github/cypress-io/cypress/54632/workflows/e04d8993-cb44-42a9-9061-e7b37bef32f2/jobs/2257912

Looks like they are missing setting the indexHtmlFile value

system-tests/project-fixtures/react/cypress/cypress-webpack-dev-server-function.config.ts Outdated Show resolved Hide resolved
npm/webpack-dev-server/cypress/e2e/webpack-dev-server.cy.ts Outdated Show resolved Hide resolved
@warrensplayer warrensplayer requested a review from a team July 18, 2023 12:13
astone123
astone123 approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@astone123 astone123 left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏻

@lmiller1990 lmiller1990 requested review from warrensplayer and a team July 20, 2023 21:03
@lmiller1990 lmiller1990 merged commit 53fd537 into develop Aug 4, 2023
79 of 80 checks passed
@lmiller1990 lmiller1990 deleted the lmiller/issue-26400 branch August 4, 2023 04:54
nagash77 pushed a commit that referenced this pull request Aug 4, 2023
@lmiller1990 @semantic-release-bot @astone123
chore: merge develop (#27468)
3685569 
* fix(cypress/webpack-dev-server): correct path for `indexHtmlFile` (#27320)

* fix: correct path for indexHtmlt

* changelog

* chore: release @cypress/webpack-dev-server-v3.5.2

[skip ci]

* chore: update external dependency paths in binary cleanup (#27460)

---------

Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com>
Co-authored-by: semantic-release-bot <semantic-release-bot@martynus.net>
Co-authored-by: Adam Stone-Lord <adams@cypress.io>
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 15, 2023

Released in 12.17.4.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v12.17.4, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Aug 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jordanpowell88 jordanpowell88 jordanpowell88 approved these changes

@astone123 astone123 astone123 approved these changes

@warrensplayer warrensplayer Awaiting requested review from warrensplayer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Custom location of cypress.config.ts crash on spec update in component tests
4 participants
@lmiller1990 @warrensplayer @jordanpowell88 @astone123