-
Notifications
You must be signed in to change notification settings - Fork 2k
Use perl
instead of replace
for dependency updates
#103216
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
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
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 find @aduth! 👍
Just a minor question on cross-platform support
sed
instead of replace
for dependency updatesperl
instead of replace
for dependency updates
@@ -138,7 +138,7 @@ | |||
"translate:calypso": "wp-babel-makepot './{client,packages,apps}/**/*.{js,jsx,ts,tsx}' --ignore '**/node_modules/**,**/test/**,**/*.d.ts' --base './' --dir './build/strings' --output './public/calypso-strings.pot'", | |||
"tsc": "NODE_OPTIONS='--max-old-space-size=4096' tsc", | |||
"typecheck": "yarn run tsc --project client", | |||
"update-deps": "rm -rf yarn.lock && yarn run distclean && yarn install && replace --silent 'http://' 'https://' . --recursive --include='yarn.lock' && touch -m node_modules", | |||
"update-deps": "rm -rf yarn.lock && yarn run distclean && yarn install && perl -pi -e s,http://,https://,g yarn.lock && touch -m node_modules", |
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.
As a side-point: I'm somewhat confused by some of what this command is doing, and how we use it in our workflows.
On what this command is doing: Is it strictly necessary to include distclean
and the touch
parts of this command? It's not clear what impact those would have on a script presumably designed to update our transitive dependencies. It's also unclear what circumstances (if any) actually result in http://
links being included, which sounds like the sort of thing that could be a bug or edge-case no longer relevant in recent versions of Yarn.
On how we use it in our workflows: While I can imagine some value in having a script like this to facilitate a task of updating transitive dependencies, I've been unable to find any documentation or internal references that mention it. I don't think it makes sense to include scripts here that aren't part of some documented procedure and require a developer to be aware of its existence through close examination of package.json
.
Personally, I'd rather take the opposite stance, and remove any and all commands that aren't directly referenced as part of CI or documented workflows. And if that disrupts someone's own personal workflow, then the onus should be on them to either make it a documented or CI-integrated process, or find another way to use it locally if it's not valuable to be common to the project.
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.
If we look at git history, can we find why this code was introduced? We may have more context and be able to make a better decision about keeping it or removing it.
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.
If we look at git history, can we find why this comment was introduced? We may have more context and be able to make a better decision about keeping it or removing it.
Good call! It looks like it has a very long history, originating back to an old Makefile task added in #5386 (reviewed by a familiar face! 😂 ) and was originally documented in a docs/shrinkwrap.md
file. I'm guessing as we switched to Yarn the documentation was lost or replaced, resulting in the command not being referenced anywhere.
The http:
stuff itself was added in #29718, which specifically dealt with a NPM issue. I'd imagine it probably doesn't even apply for Yarn.
In summary, I think we could probably just remove this command altogether.
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.
Yep, let's get rid of it
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.
Removed in #103307
Proposed Changes
replace
dependency, replacing with equivalentsed
perl
commandWhy are these changes being made?
yarn.lock
files. There's only one.Testing Instructions
I don't know the specific scenario where a
http:
URL would be added toyarn.lock
, but it's easy enough to replicate by manually editing one or morehttps
URLs inyarn.lock
tohttp
and running theperl
portion of the command to verify that the replacements are made and written in-line.Pre-merge Checklist