-
Notifications
You must be signed in to change notification settings - Fork 170
👷 Other strategy to handle test app lockfiles with renovate #4028
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
Changes from all commits
b5f0b47
92436c4
f65ef97
0bccecf
e1e9ff8
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 |
|---|---|---|
|
|
@@ -27,6 +27,23 @@ runMain(async () => { | |
| function buildApp(appPath: string) { | ||
| printLog(`Building app at ${appPath}...`) | ||
| command`yarn install --no-immutable`.withCurrentWorkingDirectory(appPath).run() | ||
|
|
||
| // install peer dependencies if any | ||
|
Collaborator
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. 💬 suggestion: Maybe add a comment explaining why we are using peed dependencies instead of dependencies. I think I got it but it's not obvious. (ideally that comment would be in the package.json, but 🤷) |
||
| // intent: renovate does not allow to generate local packages before install | ||
| // so local packages are marked as optional peer dependencies and only installed when we build the test apps | ||
| const packageJson = JSON.parse(fs.readFileSync(path.join(appPath, 'package.json'), 'utf-8')) | ||
| if (packageJson.peerDependencies) { | ||
| // For each peer dependency, install it | ||
| for (const [name] of Object.entries(packageJson.peerDependencies)) { | ||
| command`yarn add -D ${name}`.withCurrentWorkingDirectory(appPath).run() | ||
| } | ||
| // revert package.json & yarn.lock changes if they are versioned | ||
| const areFilesVersioned = command`git ls-files package.json yarn.lock`.withCurrentWorkingDirectory(appPath).run() | ||
| if (areFilesVersioned) { | ||
| command`git checkout package.json yarn.lock`.withCurrentWorkingDirectory(appPath).run() | ||
| } | ||
| } | ||
|
|
||
| command`yarn build`.withCurrentWorkingDirectory(appPath).run() | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,12 +4,22 @@ | |
| "scripts": { | ||
| "build": "webpack" | ||
| }, | ||
| "dependencies": { | ||
| "@datadog/browser-logs": "file:../../../packages/logs/package.tgz", | ||
| "@datadog/browser-rum": "file:../../../packages/rum/package.tgz" | ||
| "peerDependencies": { | ||
| "@datadog/browser-logs": "*", | ||
| "@datadog/browser-rum": "*" | ||
| }, | ||
| "peerDependenciesMeta": { | ||
| "@datadog/browser-logs": { | ||
| "optional": true | ||
| }, | ||
| "@datadog/browser-rum": { | ||
| "optional": true | ||
| } | ||
|
Comment on lines
+11
to
+17
Collaborator
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. 🥜 nitpick: If we're gonna install them anyway, then they are no optional (and it seems that npm would actually install them automatically, unfortunately this is not the case for yarn) You might need to add the peer dependencies before doing
Collaborator
Author
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. The issue is that renovate don't allow to build those dependencies before running the install to re-generate the lock file. So the idea is to have a way to run the install without having those dependencies built, otherwise the install fails as the dependencies are missing. Do you have a simpler alternative in mind?
Collaborator
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. all good then! thanks |
||
| }, | ||
| "resolutions": { | ||
| "@datadog/browser-rum-core": "file:../../../packages/rum-core/package.tgz", | ||
| "@datadog/browser-rum": "file:../../../packages/rum/package.tgz", | ||
| "@datadog/browser-logs": "file:../../../packages/logs/package.tgz", | ||
| "@datadog/browser-core": "file:../../../packages/core/package.tgz", | ||
| "@datadog/browser-rum-slim": "file:../../../packages/rum-slim/package.tgz", | ||
| "@datadog/browser-worker": "file:../../../packages/worker/package.tgz" | ||
|
|
||
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.
💭 thought: I guess this is because otherwise the type definitions from the peer dependencies are not found? That's a bit annoying to have to build just to typecheck 😞 (but I don't see an alternative)
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.
FYI, here I'm just reverting to the behavior that we add before #4019, since we don't nee to run yarn build at install anymore