-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
chore: prep npm packages for use with Cypress v10 #22205
Conversation
Thanks for taking the time to open a PR!
|
configFile = await findUp(configFiles, { cwd: projectRoot } as { cwd: string }) | ||
|
||
if (configFile) { | ||
debug('found webpack config %s', configFile) | ||
const sourcedConfig = await importModule(configFile) | ||
const sourcedConfig = configFile.endsWith('.mjs') ? await import(configFile) : require(configFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This make me nervous. Have you tried this with a project with type: module
in package.json? In my experience extension is very unreliable when determining how to import things.
Other than this one change, LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was reverting to what was there before, but I confirmed it doesn't work. Turns out the TS compilation will convert import
to a require which is not what we want (see here). There are some workarounds, I'll check both webpack and vite since we do similar things for loading the config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going over this PR now -- we can sync on this if needed, I've been dealing with module stuff lately. Not saying I have a perfect solution, but we can explore some things.
If we change the tsconfig
to "module": "esnext"
it will not generate require
, but maintain the original import
.
Actually, thinking about it, we have the exact same problem in packages/server
, where we need to grab their cypress.config
, which I've been banging my head on the wall over for the last three days. User can have a cypress.config
can be:
"type": "module"
withjs
"type": "CommonJS"
(the default) with.mjs
.ts
with "module": "CommonJS" intsconfig.json
.ts
with "module": "esnext" intsconfig.json
It's also possible to have .cjs
and, believe it or not, .mjs
and .mts
- it's pretty impractical, right now, but we need to cover as many as possible and fail as gracefully as possible.
I think we probably want to have one single, uniform way to handle "all the things" if possible. The only difference, I guess, would be that the binary ships ts-node
, so we know we can use that, but npm/webpack-dev-server
cannot rely on ts-node
been installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The obvious edge case here:
configFile = "cypress.config.ts"
const sourcedConfig = configFile.endsWith('.mjs') ? await import(configFile) : require(configFile)
Now we do require("cypress.config.ts")
, which is obviously invalid unless you are using ts-node
, which they might (probably) won't be ... require
is for CJS only, so it won't work with 1) ts
and even if it did, most TS files are using import
syntax.
packages/server
handles this with ts-node/esm
, so it'd be fine there - but for a user land module like this, they'll have a bad day. We can definitely just document what is/isn't supported, or make a recommendation to use ts-node
etc. I think we will need to think about this a bit more - what are you thoughts?
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
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 |
@@ -48,4 +47,15 @@ describe('Config options', () => { | |||
cy.contains(specWithWhitespace).click() | |||
cy.get('.passed > .num').should('contain', 1) | |||
}) | |||
|
|||
it('supports @cypress/vite-dev-server', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this test actually prove works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The system test is hooked up the use import { devServer } from '@cypress/vite-dev-server
via the file:
package alias, so it's a test to prove that importing this as a standalone package and using the function signature works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some general comments - I'm wondering if we don't revert the module loading changes you made and do them separately (eg, this ticket was just about "release the things" not necessarily fixing bugs?
npm/vite-dev-server/package.json
Outdated
@@ -17,8 +16,7 @@ | |||
}, | |||
"dependencies": { | |||
"debug": "4.3.3", | |||
"find-up": "6.3.0", | |||
"local-pkg": "0.4.1", | |||
"find-up": "5.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, but I really like the idea of moving (slowly) towards ES Modules.
I wonder what the logistics of:
- Write
vite-dev-server
using"type": "module"
(withtsconfig.json
using"module": "esnext"
) - Now the package is 100% authored using ES modules.
- Use rollup to generate a CJS and ESM bundle.
Haven't looked much into ESM source -> ESM/CJS combined output. I know a bunch of modules like find-up
just ripped the bandaid off an took a hard "ESM only" stance. While we cannot reasonably do that, the idea of at least authoring in ES Modules is appealing, and pushing the CommonJS concern to a build-step-only thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to consider here is that these packages are always going to be consumed through our loader process since they are imported in the config (which is processed in the child process we spin up). Not quite sure of all the implications of this, but if we always transpile the config to commonjs then it might factor into how we decide to bundle these packages.
I'm seeing the need for a clearly defined tech-brief just talking through all of this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, we'd need to change our development workflow to transpire this on the fly if we wanted to write pure ESM (probably not a big deal, but more work than really makes sense now).
That said, unless we eventually do commit to it, we will likely be stuck on ESM forever (like most of the ecosystem). We finally got the module system we all dreamed off, but it came 10 years too late :(
configFile = await findUp(configFiles, { cwd: projectRoot } as { cwd: string }) | ||
|
||
if (configFile) { | ||
debug('found webpack config %s', configFile) | ||
const sourcedConfig = await importModule(configFile) | ||
const sourcedConfig = configFile.endsWith('.mjs') ? await import(configFile) : require(configFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The obvious edge case here:
configFile = "cypress.config.ts"
const sourcedConfig = configFile.endsWith('.mjs') ? await import(configFile) : require(configFile)
Now we do require("cypress.config.ts")
, which is obviously invalid unless you are using ts-node
, which they might (probably) won't be ... require
is for CJS only, so it won't work with 1) ts
and even if it did, most TS files are using import
syntax.
packages/server
handles this with ts-node/esm
, so it'd be fine there - but for a user land module like this, they'll have a bad day. We can definitely just document what is/isn't supported, or make a recommendation to use ts-node
etc. I think we will need to think about this a bit more - what are you thoughts?
274b700
to
722e838
Compare
@lmiller1990 @BlueWinds I decided to revert the changes made to how the webpack config was being loaded. I wrote a decent amount of test cases to cover all the variations of js + cjs + ts + esm and they were pros and cons for each approach. The way it is wired up now is actually the best for our use case of providing good support for the most common cases (js + ts + cjs). I've learned that there is a workaround for esm and ts edge-cases which is to import the webpack config into the cypress config i.e. not rely on We have some work to do for handling esm, and I'm going to push up my test cases and link them to a new issue where we can properly work through adding first class support for esm. I feel better having this issue just be related to publishing the packages rather than solve the nightmare that is support for cjs + esm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, good call on not doing too deep in the module stuff. #21939 seems to be working pretty well, so we can expand on that approach moving forward. Let's work together on a brief to figure this out once and for all.
Windows CI is fixed in develop, you might need to wait for develop to merge into master before this is able to be merged.
722e838
to
77cfadd
Compare
Going to merge today, there is a CI issue that I'm merging to develop and will get merged into master soon: #22289 |
BREAKING CHANGE: new version of packages for Cypress v10
77cfadd
to
868081a
Compare
Percy-finalize having an issue, all test are passing so going to merge. |
User facing changelog
Publish Cypress v10 compatible npm packages
Additional details
Removes
"private": true
from the npm packages that we had disabled and fixes a few bugs in our implementation that would have caused issues. Notable changes:@cypress/webpack-batteries-included-preprocessor
in webpack-dev-server from__dirname
tocypressBinaryRoot
. Relying on__dirname
only works when sourcing from within the binary (or sourcing from the packages installed node_modules).find-up
to 5.0.0 as >6.0.0 is ESM only. We were relying onlocal-pkg
to import it which usesrequire
. Removed both packages.The scope for the original issue became very bloated so I'm not ticking all the boxes from that issue (such as linting non-used deps or releasing as
RC
). Not sure why we came to the decision to publish them as RC but I thoroughly tested all the packages locally and I'm confident we can go forward with a normal breaking change.I ran the
npm-release.js
script locally and the new versions are as expected:We should merge with care. I included two commits, one with all the changes to the files in
npm/
with theBREAKING CHANGE
commit message and another with the rest of the changes. I didn't want to include aBREAKING CHANGE
commit message that touched files outside of npm. When merging, I'd like to not squash.Need to follow up with DX about documentation. There is a section about custom-dev-servers, gotta check if this needs to be expanded.
Steps to test
I tested the packages by running
yarn pack
and installing the generated.tgz
. I added a few system tests for the dev-servers to verify the use of the function signature.How has the user experience changed?
na
PR Tasks
cypress-documentation
?type definitions
?