-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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: patch-package is not applied in dist'ed build #19239
Changes from all commits
42c49c8
2e09a78
20ef207
a91681b
bffada6
f605e86
38ad09c
5bfb385
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,8 @@ | |
}, | ||
"dependencies": { | ||
"circular-json": "0.5.9", | ||
"engine.io": "5.0.0", | ||
"engine.io-parser": "4.0.2", | ||
Comment on lines
+17
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needed to add this, as it was being hoisted when devDeps are stripped & patch wasn't being applied. |
||
"socket.io": "4.0.1", | ||
"socket.io-client": "4.0.1" | ||
}, | ||
|
@@ -30,6 +32,8 @@ | |
], | ||
"workspaces": { | ||
"nohoist": [ | ||
"engine.io", | ||
"engine.io-parser", | ||
"socket.io", | ||
"socket.io/socket.io-parser", | ||
"socket.io-client", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,15 +68,23 @@ export async function buildCypressApp (options: BuildCypressAppOpts) { | |
// Copy Packages: We want to copy the package.json, files, and output | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why re-name from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do the patches in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the way you're supposed to name them for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahhhh okay 👍🏻 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, had the wrong link, updated the above with the failing circle job. But here's the docs: https://github.com/ds300/patch-package#dev-only-patches And no, the socket & server ones are prod dependencies, so they remain |
||
log('#copyAllToDist') | ||
await packages.copyAllToDist(DIST_DIR) | ||
fs.copySync(path.join(CY_ROOT_DIR, 'patches'), path.join(DIST_DIR, 'patches')) | ||
|
||
const jsonRoot = fs.readJSONSync(path.join(CY_ROOT_DIR, 'package.json')) | ||
|
||
fs.writeJsonSync(meta.distDir('package.json'), _.omit(jsonRoot, [ | ||
'scripts', | ||
const packageJsonContents = _.omit(jsonRoot, [ | ||
'devDependencies', | ||
'lint-staged', | ||
'engines', | ||
]), { spaces: 2 }) | ||
'scripts', | ||
]) | ||
|
||
fs.writeJsonSync(meta.distDir('package.json'), { | ||
...packageJsonContents, | ||
scripts: { | ||
postinstall: 'patch-package', | ||
}, | ||
}, { spaces: 2 }) | ||
|
||
// Copy the yarn.lock file so we have a consistent install | ||
fs.copySync(path.join(CY_ROOT_DIR, 'yarn.lock'), meta.distDir('yarn.lock')) | ||
|
@@ -153,7 +161,7 @@ require('./packages/server')\ | |
log(`#testVersion ${meta.distDir()}`) | ||
await testVersion(meta.distDir(), version) | ||
|
||
// testBuiltStaticAssets | ||
log('#testStaticAssets') | ||
await testStaticAssets(meta.distDir()) | ||
|
||
log('#removeCyAndBinFolders') | ||
|
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.
why did this need move from a dev-dep?
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.
Trying to find where it failed - might have only been locally when I was writing the test case for the patch being applied, and it wasn't... and then I noticed that we do indeed use the package in a prod situation:
cypress/packages/server/lib/util/ts_node.js
Lines 30 to 37 in 11e99fc
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.
Nice catch then!