Skip to content

move dependencies from monorepo root to correct packages#4550

Open
Igloczek wants to merge 1 commit intoalpinejs:mainfrom
Igloczek:move-dependecies-from-root
Open

move dependencies from monorepo root to correct packages#4550
Igloczek wants to merge 1 commit intoalpinejs:mainfrom
Igloczek:move-dependecies-from-root

Conversation

@Igloczek
Copy link

package specific dependencies should always be defined inside the package, not in the monorepo root

in normal npm package scenario this would lead to the errors, but since all stuff, including external dependencies is bundled, esbuild can "borrow" the missing deps from the root, hiding the underlying problem

this PR is meant to move the deps into their correct places, so later on, hopefully, the bundling can be disabled, and the npm packages, will become actual npm packages, not CDN-ish builds, but served using NPM

@calebporzio
Copy link
Collaborator

PR Review: #4550 — move dependencies from monorepo root to correct packages

Type: Refactor
Verdict: Merge

What's happening (plain English)

  1. @alpinejs/anchor imports @floating-ui/dom in its source code. @alpinejs/sort imports sortablejs in its source code.
  2. But neither package declared these as dependencies in their own package.json. Instead, they were listed as devDependencies in the monorepo root.
  3. This works today because esbuild bundles everything (all deps get inlined into dist files), and npm workspaces hoist all deps to the root node_modules anyway — so esbuild can find them regardless of where they're declared.
  4. This PR moves @floating-ui/dom into packages/anchor/package.json and sortablejs into packages/sort/package.json, removing them from the root.

The result: if someone npm install @alpinejs/anchor and tries to use the ESM/CJS module build (not the CDN bundle), @floating-ui/dom will actually be installed as a transitive dependency. Today it silently isn't, which would cause a runtime error in that scenario.

Other approaches considered

  1. Leave as-is — Since esbuild bundles everything, this technically doesn't break anything today. But it's wrong metadata, and it means the published npm packages are broken for non-CDN usage. Not a real alternative.
  2. Move to dependencies AND mark as external in esbuild — This would be the "real" npm package fix the contributor alludes to. But that's a much bigger change (affects bundle output, CDN builds, etc.) and is rightly a separate effort.
  3. This PR's approach — Just fix the metadata. Minimal, correct, zero behavior change. Right call.

Changes Made

No changes made. The PR is clean as-is.

Test Results

  • No test files in the diff (package.json config change — tests not required)
  • CI: build passes ✅
  • Local build verified: all 12 packages build successfully with deps resolving from their new locations

Code Review

  • packages/anchor/package.json: "dependencies": {}"dependencies": { "@floating-ui/dom": "^1.5.3" } — correct, matches the import in packages/anchor/src/index.js:1
  • packages/sort/package.json: added "dependencies": { "sortablejs": "^1.15.2" } — correct, matches the import in packages/sort/src/index.js:1
  • Root package.json: both deps removed from devDependencies — correct
  • package-lock.json: "dev": true flags removed from @floating-ui/* and sortablejs entries — correct, they're now regular deps
  • No dist files in the diff ✅
  • No unrelated changes ✅
  • Verified no other root devDependencies need the same treatment — the remaining ones (axios, chalk, cypress, esbuild, jest, vitest, etc.) are all build/test tools used only by root scripts, not package source code

Security

No security concerns identified.

Verdict

Merge. This is a correct, minimal metadata fix. It moves two dependencies to where they belong — the packages that actually import them. Zero behavior change today (esbuild bundles everything), but it fixes the published npm package metadata so @alpinejs/anchor and @alpinejs/sort would actually work if someone tried to use them as proper npm modules without a bundler.

The contributor's framing about "hopefully disabling bundling later" is aspirational and not this PR's concern — but the fix itself stands on its own merits as correct package metadata.


Reviewed by Claude

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants