-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix: change default options for sourceMaps inside WBIP #31270
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
Conversation
45d60b9
to
4b24b40
Compare
cypress
|
Project |
cypress
|
Branch Review |
fix/source_maps_typescript_wbip
|
Run status |
|
Run duration | 18m 41s |
Commit |
|
Committer | AtofStryker |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
9
|
|
1233
|
|
0
|
|
32129
|
View all changes introduced in this branch ↗︎ |
UI Coverage
46.44%
|
|
---|---|
|
183
|
|
163
|
Accessibility
92.57%
|
|
---|---|
|
3 critical
8 serious
2 moderate
2 minor
|
|
883
|
4b24b40
to
b1faca0
Compare
bf2e9df
to
d9da2c8
Compare
6c14a81
to
bb9cf16
Compare
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.
we aren't doing anything with this until #31282 except feeding the webpack plugin the tsconfig file path
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.
delegated to @cypress/webpack-preprocessor
npm/webpack-preprocessor/index.ts
Outdated
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.
any idea if webpack has some type of config utility parsing? This is a bit tedious
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.
Unfortunately, not that I can find. I was hoping to find a JOI or Zod schema, but wasn't able to find anything obvious
bb9cf16
to
78e1b34
Compare
cli/CHANGELOG.md
Outdated
|
||
- [`Cypress.stop()`](https://on.cypress.io/cypress-stop) is now available to stop the Cypress App on the current machine while tests are running. This can be useful for stopping test execution upon failures or other predefined conditions. Addresses [#518](https://github.com/cypress-io/cypress/issues/518). Addressed in [#31225](https://github.com/cypress-io/cypress/pull/31225). | ||
|
||
**Bugfixes:** |
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.
@AtofStryker Need to move this to a new release.
"debug": "^4.3.4", | ||
"domain-browser": "^4.22.0", | ||
"events": "^3.3.0", | ||
"fs-extra": "^9.1.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.
🎉
npm/webpack-preprocessor/index.ts
Outdated
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.
Unfortunately, not that I can find. I was hoping to find a JOI or Zod schema, but wasn't able to find anything obvious
npm/webpack-preprocessor/index.ts
Outdated
} | ||
|
||
if (_.isObject(rule.use) && rule.use.loader && rule.use.loader.includes('ts-loader')) { | ||
// it's an Object |
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 sure this comment is really that helpful...
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.
i need to add some better comments in here as to what we are checking for
|
||
debug('typescript modified createProgram options %o', options) | ||
|
||
// @ts-ignore |
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.
// @ts-ignore | |
// @ts-expect-error |
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.
I think I can remove this entirely!
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.
Looks good overall! Added a few comments.
78e1b34
to
1b0b5ec
Compare
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Eval
ed UserInvocationStack is sometimes off for projects using Typescript 5 #29614Additional details
When updating the monorepo to TypeScript 5 last year, I noticed that the error code frames source maps did not seem quite right. We filed #29614 with the intent to fix it. It turns out that
inlineSourceMap
is what is responsible for this inaccuracy, as it is inlining the transpiled TypeScript into the spec file and not the pure TypeScript source map. To fix this, we are enablingsourceMap
totrue
for all user configs that are using@cypress/webpack-preprocessor
(@cypress/webpack-batteries-included-preprocessor
gets this via downstream impact as it uses@cypress/webpack-preprocessor
) and settinginlineSourceMap
tofalse
.This does NOT fix the custom preprocessor use case, for which we are adding documentation in cypress-io/cypress-documentation#6122 to guide users on how to configure their cypress
tsconfig.json
.This is mainly due to it being very difficult to hook into the TypeScript API to override defaults. The reasoncreateProgram
inside@cypress/webpack-preprocessor
was not getting called in TypeScript 5 was because the package is now a true ES Module and exports are no longer modifiable (which is the case with theesbuild
implemented specification). I have modified@cypress/webpack-preprocessor
to check for the presence of TypeScript 4 to try and patchcreateProgram
, but avoid it for newer versions as we know it will not be successful. I have also improved the logging around@cypress/webpack-preprocessor
to help diagnose further issues if needed.Ideally, we want to find a way to accomplish this in all cases, but for now we are mitigating through the default preprocessor and guiding users on how to configure their source maps. For instance, this doesn't work if the user is on TypeScript 5 and is using
babel-loader
with the@babel/preset-typescript
configuration. The user in this case would need to setsourceMap
in their actual config.On a semi related note, we only pass a subset of options to
ts-loader
, where we should be sending the full user config. Since this change is fairly disruptive, we will be including it in Cypress 15 with #31282Steps to test
run the tests in the project with the default
@cypress/webpack-batteries-included-preprocessor
/@cypress/webpack-preprocessor
or run a standalone project with TypeScript configured and have a command fail inside your cypress test. The codeframe should now be much more accurateHow has the user experience changed?
PR Tasks
cypress-documentation
? chore: add documentation on recommended TypeScript sourcemap option cypress-documentation#6122type definitions
?