Skip to content

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 8, 2025
+7 −220
Merged

Use perl instead of replace for dependency updates #103216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8393c3e
Use `sed` instead of `replace` for dependency updates
aduth May 7, 2025
4db441a
Use perl instead of sed for Linux compatibility
aduth May 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member Author

@aduth aduth May 8, 2025 •
edited
Loading

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.

Copy link
Contributor

@ciampo ciampo May 8, 2025 •
edited
Loading

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.

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in #103307

"vscode:link": "node bin/set-up-vs-code.js",
"vscode:unlink": "node bin/set-up-vs-code.js --unlink",
"whybundled": "NODE_ENV=production CALYPSO_ENV=production EMIT_STATS=withreasons CONCATENATE_MODULES=false yarn run build-client && whybundled client/stats.json",
Expand Down Expand Up @@ -305,7 +305,6 @@
"react-refresh": "^0.17.0",
"readline-sync": "^1.4.10",
"recursive-copy": "^2.0.14",
"replace": "^1.1.5",
"resize-observer-polyfill": "^1.5.1",
"sass": "1.54.0",
"sass-loader": "^13.3.3",
Expand Down
Loading
Loading