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

feat: add new webpack-dev-server package based on object API & bundling #20861

Merged
merged 28 commits into from
Apr 5, 2022

Conversation

tgriesser
Copy link
Member

@tgriesser tgriesser commented Mar 31, 2022

User facing changelog

Adds support for the new object syntax for the webpack-dev-server. Adds a new webpack-dev-server-fresh package to manage this support, it will replace the existing webpack-dev-server once all frameworks are supported.

Ported unit tests from webpack-dev-server, and added additional unit tests covering new functionality. Adds "open mode" tests for the matrix of supported webpack / webpack-dev-server versions, including recovering from failures, and adds system-tests.

Adds the following system test improvements:

  • Adds the concept of project-fixtures for better testing the same project against multiple dependency versions documentation added here
  • For open-mode system tests, automatically tries updating the yarn.lock in the project directory if the --frozen-lockfile install fails, and then rm node_modules in the project directory, and retry once, so you just have to bump the package.json version and it'll work without needing to do all that manually: link (will update to use fs.remove)

Also marked the webpack-dev-server-fresh package.json with internal: true for now to ensure it doesn't accidentally get published.

PR Tasks

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

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 31, 2022

Thanks for taking the time to open a PR!

* 10.0-release:
  fix: add index.mjs to the published files of cli (#20884)
  refactor: lift indexHtmlFile up to component, add validation (#20870)
  fix: allow migration of pluginsFile using `env` properties (#20770)
  fix: viewport from CLI on CT (#20849)
  fix: git data source unit test failure (#20875)
  fix: Ensuring current browser is synchronized between app and launchpad (#20830)
  Fix missed await on merge conflict resolution
  test(unification): move record keys to contexts (#20860)
  test: move record keys to contexts (#20859)
  make alerts more responsive
  chore: Update Chrome (stable) to 100.0.4896.60 (#20841)
  Revise test.
  fix: cy.root respect timeout option.
  fix(deps): update dependency ansi-regex to v4.1.1 [security] (#20836)
  chore(deps): update dependency ansi-regex to 4.1.1 [security] (#20807)
  chore: Refactor cri-client to use async/await (#20825)
  remove automationId from runnerStore
  fix firefox automation and adress feedback
  feat: add automation warning/disconnected states in app
*
* @internal
*/
export function sourceRelativeWebpackModules (config: WebpackDevServerConfig) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is new functionality, it ensures we're resolving the webpack/webpack-dev-server/ html-webpack-plugin relative to the project root, for better compat with the user's webpack config.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do something similar in the launchpad when we do the initial setup and find your deps, see here, using resolve-from.

Would resolve-from be a candidate for finding the nearest module here, or is this a bit too custom/different?

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like resolve-from is providing behavior that is now provided in Node's require.resolve with the paths option: sindresorhus/resolve-from#8 since v8.7

I think most other places in the codebase where we do scoped requires, we utilize this pattern, rather than depending on a package, so I'd lead toward explicitness here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. Maybe we should not bother with resolve-from and just use require.resolve in the other places, too.

@@ -174,7 +175,7 @@ beforeEach(() => {
taskInternal('__internal__beforeEach', undefined)
})

function scaffoldProject (projectName: ProjectFixture, options: { timeout?: number} = {}) {
function scaffoldProject (projectName: ProjectFixture, options: { timeout?: number } = { timeout: SIXTY_SECONDS }) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Bumped this due to the yarn occasionally taking a bit longer than 10 seconds.

Comment on lines +65 to +74
const packageJson = require(`${to}/package.json`)
const fixtureDir = packageJson.projectFixtureDirectory

if (fixtureDir) {
if (!projectFixtureDirs.includes(fixtureDir)) {
throw new Error(`Invalid project fixture directory: ${fixtureDir}, expected one of ${projectFixtureDirs}`)
}

await fs.copy(_path.join(projectFixtures, fixtureDir), to)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Implementation of the new projectFixtureDirectory feature

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixtures will be an awesome feature for our system tests 💯

Comment on lines +39 to +43
cy.withCtx(async (ctx) => {
await ctx.actions.file.writeFileInProject(`src/MissingReact.jsx`,
`import React from 'react';
${await ctx.actions.file.readFileInProject('src/MissingReact.jsx')}`)
})
Copy link
Member Author

Choose a reason for hiding this comment

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

Tests in open mode that the webpack-dev-server is able to auto-refresh when we fix issues in the user code.

Copy link
Contributor

Choose a reason for hiding this comment

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

New infra 💯

I think this is a good one to demo next biweekly demo - if we don't show this stuff off and champion it, most people won't know. I think a lot of people are not using system-tests in the most productive way for this reason.

Comment on lines +50 to +55
cy.contains('MissingReactInSpec.cy.jsx').click()
cy.get('.failed > .num').should('contain', 1)
cy.withCtx(async (ctx) => {
await ctx.actions.file.writeFileInProject(`src/MissingReactInSpec.cy.jsx`,
await ctx.actions.file.readFileInProject('src/App.cy.jsx'))
})
Copy link
Member Author

Choose a reason for hiding this comment

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

Tests in open mode that the webpack-dev-server is able to auto-refresh when we fix issues in the spec.

packages/frontend-shared/cypress/e2e/e2ePluginSetup.ts Outdated Show resolved Hide resolved
@tgriesser tgriesser marked this pull request as ready for review April 1, 2022 16:02
@tgriesser tgriesser requested review from a team as code owners April 1, 2022 16:02
@tgriesser tgriesser requested review from jennifer-shehane, flotwig, JessicaSachs, ZachJW34 and tbiethman and removed request for a team April 1, 2022 16:02
@jennifer-shehane jennifer-shehane removed their request for review April 1, 2022 16:03
@@ -12,7 +12,7 @@ import { getStackLines, replacedStack, stackWithoutMessage, splitStack, unsplitS

const whitespaceRegex = /^(\s*)*/
const stackLineRegex = /^\s*(at )?.*@?\(?.*\:\d+\:\d+\)?$/
const customProtocolRegex = /^[^:\/]+:\/+/
const customProtocolRegex = /^[^:\/]+:\/{1,3}/
Copy link
Member Author

Choose a reason for hiding this comment

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

Will open a separate PR for this

Copy link
Member Author

Choose a reason for hiding this comment

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

@tgriesser tgriesser requested a review from tbiethman April 4, 2022 23:14
tbiethman
tbiethman previously approved these changes Apr 4, 2022
lmiller1990
lmiller1990 previously approved these changes Apr 4, 2022
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.

I was not able to attend the sync w/ @tgriesser and @ZachJW34 but seems like everything is 👌 , none of my comments are blockers.

Let's get this ✔️ and resolve conflicts and merged 💯

ZachJW34
ZachJW34 previously approved these changes Apr 5, 2022
Copy link
Contributor

@ZachJW34 ZachJW34 left a comment

Choose a reason for hiding this comment

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

Once were green LGTM!

ZachJW34
ZachJW34 previously approved these changes Apr 5, 2022
tbiethman
tbiethman previously approved these changes Apr 5, 2022
@tgriesser tgriesser dismissed stale reviews from tbiethman and ZachJW34 via fe7a28d April 5, 2022 15:16
@flotwig flotwig requested review from tbiethman, ZachJW34 and BlueWinds and removed request for flotwig April 5, 2022 15:32
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.

None yet

4 participants