Stop declaring yaml directly in theme (use already-present js-yaml) - #7721
Stop declaring yaml directly in theme (use already-present js-yaml)#7721amcaplan wants to merge 1 commit into
yaml directly in theme (use already-present js-yaml)#7721Conversation
7b1aeb1 to
0638348
Compare
c5ca3ac to
c772d26
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates @shopify/theme to stop using the yaml package and instead generate YAML output via js-yaml’s dump, aiming to reduce direct dependency churn in the theme package.
Changes:
- Replace
yaml’sstringifyusage in theme-check CLI output withjs-yaml’sdump. - Update
packages/theme/package.jsondependencies/devDependencies accordingly. - Regenerate
pnpm-lock.yamlto reflect dependency changes (and additional lockfile normalization).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pnpm-lock.yaml |
Adds js-yaml / @types/js-yaml for packages/theme and updates lock entries. |
packages/theme/src/cli/services/check.ts |
Switches YAML serialization calls from yaml to js-yaml. |
packages/theme/package.json |
Replaces yaml dependency with js-yaml and adds @types/js-yaml. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import {dump as yamlDump} from 'js-yaml' | ||
|
|
||
| type OffenseMap = Record<string, Offense[]> |
There was a problem hiding this comment.
Thanks for flagging — I checked this carefully and it's safe as written.
js-yaml@4.1.1 (the version we pin to) is dual-package, not CJS-only. Its exports map resolves the import condition to a real ESM build:
"exports": { ".": { "import": "./dist/js-yaml.mjs", "require": "./index.js" } }and dist/js-yaml.mjs ends with a genuine named export — export { ..., dump, load, loadAll, ... }. So Node's ESM loader selects the .mjs directly and dump is a true named export; there's no CJS named-export heuristic in play and no "Named export 'dump' not found" risk.
Verified at runtime — a pure-ESM import { dump as yamlDump } from 'js-yaml' from this package resolves 4.1.1 with typeof dump === 'function' and produces correct YAML.
Worth noting the concern would have applied to 4.1.0, which is pure CJS — 4.1.1 is the release that added the ESM build, and we pin to 4.1.1 exactly.
Co-Authored-By: Claude <noreply@anthropic.com> 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
c772d26 to
d7e7687
Compare
gonzaloriestra
left a comment
There was a problem hiding this comment.
What's the benefit of this change? We are not removing a dependency, but adding a new one 🤔
yamlis still a transitive dependency of 8 packagesjs-yamlwas a transitive dependency only for dev, but now we are adding it as a new direct runtime dependency
|
@gonzaloriestra fair question, and you're right that this isn't a true removal — the title oversells it. Why dropping the direct declaration achieves that:
On
I'll retitle to something like "Stop declaring 🤖 This reply was drafted by Claude (AI-generated) and reviewed by me. |
yaml directly in theme (use already-present js-yaml)
|
Again: "56 bumps / 24mo" is not right. Yaml is grouped with other deps in PRs like this that are not very frequent. All these PRs are based on that "Dependabot churn" that is actually not a problem in my opinion... About this one, I was wrong about |
|
@gonzaloriestra we have a weekly dependabot job that seems to bump everything that is a direct (not transitive) dependency. The less noise there is in those PRs, the more confident we can feel just keeping everything up to date... and the less far behind we are when inevitably some package has a security issue and we are forced to catch up immediately. |
|
That PR includes upgrades for 40 small dependencies. With this change, instead of upgrading So if we are able to completely remove a dependency, great. But I don't see the point of just switching one for another if it's working well. |
|
This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. |
Stops
@shopify/themefrom declaringyamlas a direct dependency, which is the source of the recurring Dependabot version-update churn (56 bumps / 24mo —themewas the only package in the repo declaring it directly).This is not a tree-level removal:
yamlremains a transitive dependency via@shopify/theme-check-node, so it stays in the lockfile and a future security advisory could still trigger a PR. The win is narrower — eliminating the routine weekly bumps, since npm version updates only touch deps explicitly declared in a manifest.The
YAML.stringifycalls in theme-check output are switched tojs-yaml'sdump.js-yamlis not new to the runtime tree — it's already a production transitive dep ofthemevia@shopify/cli-kit → @apidevtools/json-schema-ref-parser(js-yaml: ^4.1.0), so nothing extra is installed. It's also far more stable (~2 releases in 4 years vs. ~18 foryamlsince mid-2024), so it won't reproduce the churn.Net direct runtime deps for
theme: unchanged (one-for-one swap). The only true addition is the dev-only@types/js-yaml.Validation:
🤖 Automated dependency-removal initiative — AI-generated draft, needs human review.