[dependencies-replacement] Update package-json to latest, removing transitive dependencies #231

VanTanev · 2024-10-14T16:52:20Z

References #221

Upgrading package-json removes 21 dependencies. Necessitated changing node requirement to >=18.18.0 in @manypkg/cli.
Then upgrading semver and sembear removes 2 dependencies, by making all 3 resolve to the same version of semver.

Total: 23 dependencies removed.

Additional changes

Upgraded @preconstruct/cli and @babel/* dev dependencies, because I initially thought they were the cause of dynamic import not working with preconstruct dev, but it turned out that a config change was necessary in babel.config.js. I can revert this if you prefer, but it seems to work OK with the latest versions.
Upgraded fsevents transitive dev dependency because the current version was broken on M1 Macs.

changeset-bot · 2024-10-14T16:52:23Z

🦋 Changeset detected

Latest commit: 4f534b0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@manypkg/cli	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

VanTanev · 2024-10-14T16:55:07Z

Should I be squashing the changeset commit into the commit that has the actual changes?

Andarist · 2024-10-14T16:56:29Z

Should I be squashing the changeset commit into the commit that has the actual changes?

It will be squashed when merging so it doesn't matter

Andarist · 2024-10-14T17:02:03Z

packages/cli/src/upgrade.ts

+    const scope = packageName.split("/")[0];
+    const registryUrl_ = registryUrl(scope);
+    const packageUrl = new URL(
+      encodeURIComponent(packageName).replace(/^%40/, "@"),
+      registryUrl_
+    );
+    const authInfo = registryAuthToken(registryUrl_.toString(), {
+      recursive: true,
    });
+
+    const headers: Record<string, string> = {
+      accept:
+        "application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*",
+    };
+    if (authInfo) {
+      headers.authorization = `${authInfo.type} ${authInfo.token}`;
+    }
+
+    const response = await fetch(packageUrl, { headers });


This is totally something I'd prefer not to have in the code 😢 I'm happy to offload this kind of code to dependencies just so I can stay ignorant of the requirements for fetching this thing.

At the very least, this should be moved to a separate function and some resources should be added in the comment to explain what's happening (I suspect most of this comes from the package-json package, if that's the case we should have proper license attribution for that package).

Is package-json maintained? Could PRs be open against that project to shrink its size?

It is maintained, but it's ESM-only and node >=18.18.0 only.

The latest version is significantly smaller, because it uses ky instead of got for network requests.

Unfortunately, with our version requirements, the above is the smallest possible solution.

The CLI package here is still on 0.x version and you have already scheduled some minor releases for it (which are effectively treated as breaking for 0.x packages). Let's bump the required node version for the CLI alone

What about package-json being ESM-only? I don't know of a clean solution, other than converting the cli package to ESM.

the dynamic import should work fine here. You might have to use keepDynamicImportAsDynamicImportInCommonJS though:
https://github.com/preconstruct/preconstruct/blob/54c412d6f10752e3e4d0aa04aced1c44958baee1/packages/cli/src/project.ts#L31

Since this is a CLI you could also convert it to ESM but this will likely be more complicated given the test setup, tsconfigs and all. So the dynamic import parth forward (for now) is probably just simpler

Roger, will look into this!

(it honestly slipped my mind for a moment that dynamic imports were a thing)

OK, I needed to change babel.config.js to make preconstruct dev work. Other than that, using dynamic import() was absolutely the correct call, thanks!

Should be good for re-review, I've also updated the PR description.

Andarist · 2024-10-15T21:02:25Z

.changeset/olive-tomatoes-provide.md

+"@manypkg/cli": minor
+---
+
+Node requiremet raised to `node >= 18.18.0`. Upgraded `package-json`, `semver` and `sembear` dependencies.


the updated dependencies should be one changeset file and the bumped node requirement should be another. Even though they are introduced by a single PR they will like 2 separate changes for the end user

Thanks, I'm not used to writing changesets so this is very helpful!

Andarist · 2024-10-15T21:03:30Z

babel.config.js

+        // necessary to preserve dynamic import of ESM-only modules
+        exclude: ["proposal-dynamic-import"],


have you checked the build output? i'd expect that preconstruct flag I mentioned to be required too - although I never really used it myself so far

The preconstruct flag was already set, and the binary worked correctly with yarn preconstruct build but not with yarn preconstruct dev until the babel change was made.

ah cool, I see we are using it since #181

I wonder if this would still be required if we'd update all of the Babel deps. Maybe we just have an outdated "stable features" list in the @babel/preset-env since we are using an old version?

Andarist · 2024-10-15T21:03:48Z

babel.config.js

@@ -4,6 +4,8 @@ module.exports = {
      "@babel/preset-env",
      {
        targets: { node: 14 },


this should be updated now :p

ah, maybe it shouldnt - since we are only bumping the node version for the CLI itself

I actually don't know @preconstruct/cli's exact behavior here - does the build downlevelling use babel or TSConfig to decide what to output? I would have thought it's TSConfig (esp given the different code behavior in preconstruct dev vs preconstruct build - it seems like preconstruct build does not consider babel.config.js)

Runtime files are exclusively~ transformed by Babel

it seems like preconstruct build does not consider babel.config.js

It sounds like a configuration issue or a bug in Preconstruct. How did you come up with that conclusion? What I could try to repro locally and how I could observe the broken output?

In @manypkg/cli upgrade package-json@^10.0.1.

yarn install

yarn preconstruct dev

yarn manypkg check # broken

yarn preconstruct build

yarn manypkg check # works

It seems like the rollup setting that would control build output is outputs.generatedCode. It doesn't seem like preconstruct sets this explicitly, which probably means that the build uses the rollup defaults:

https://github.com/rollup/rollup/blob/v2.79.2/src/utils/options/normalizeOutputOptions.ts#L346-L363

Which should ultimately default to es5: https://github.com/rollup/rollup/blob/master/src/utils/options/options.ts#L181-L187

nice github digging, my style - i love it :) The second link might be slightly inaccurate as it points to master an not to the tagged version but I doubt it matters here much (pro tip - you can hit y to get a permalink in the URL bar)

It seems like the rollup setting that would control build output is outputs.generatedCode. It doesn't seem like preconstruct sets this explicitly, which probably means that the build uses the rollup defaults:

I'm slightly confused here given the plugin in Preconstruct that you linked to. But tbh this might just be the issue of not checking out locally how it behaves and what's the actual current output. Given you already looked into it - do you think that anything has to be fixed in Preconstruct itself?

I'd treat the configuration issue with Babel for preconstruct dev purposes separately. This might not be something to fix in Preconstruct. So I'm primarily interested in the effect of all of this on preconstruct build

Yeah, the second link is to master, but it's the same for version 2.79.2.

As for preconstruct... I don't think preconstruct is doing anything wrong, per se. It's not obvious that you need to configure 2 separate settings for build and dev to execute in the same way, but it does make some sense. (tangentially, keepDynamicImportAsDynamicImportInCommonJS would be easier to implement in rollup@3, because it's now a separate setting that defaults to true, so preconstruct wouldn't need a custom plugin.)

On the more broad question of what the output of preconstruct build should be - there doesn't seem to be a way to configure preconstruct to output more modern JS. ES2015 is fully supported on node >= 10.15.0 (and node 8 is missing only like 2 methods), but the rollup es2015 preset isn't even the full ES2015 standard. I think it would make sense to change preconstruct build output to use the the es2015 rollup preset), as those parts should be node >= 8.0.0 I think

Preconstruct could also consider enabling keepDynamicImportAsDynamicImportInCommonJS: true by default, but that requires node >= 14.0.0 so it's a harder config change.

The Rollup output options aren't really relevant, they're not for down-levelling, they only control a small amount of helper code that Rollup sometimes needs to emit. Updating the target in Babel will make preconstruct build output more modern JS

Ah. Apologies then, I was completely wrong.

…ic import() statements This removes 21 dependencies from @manypkg/cli

Andarist · 2024-10-17T09:33:25Z

packages/cli/package.json

@@ -31,6 +31,6 @@
    "strip-ansi": "^6.0.0"
  },
  "engines": {
-    "node": ">=14.18.0"
+    "node": ">=18.18.0"


I think we could just use "node": ">=18.0.0". 18.18.0 is a weird threshold, it's neither the current latest 18 nor it's the one that when 18 was marked as LTS for the first time (that was 18.12.0).

It is weird. I thought we needed that patch version for some reason, but looking at the changelog I'm completely wrong.

I think that we should be fine setting it at "node": ">=18.0.0", I don't see a reason we would need the LTS specifically.

VanTanev force-pushed the dependencies-replace-package-json branch from 6ea6b64 to 11a8145 Compare October 14, 2024 16:54

Andarist reviewed Oct 14, 2024

View reviewed changes

VanTanev force-pushed the dependencies-replace-package-json branch 3 times, most recently from 56846b6 to 2255082 Compare October 15, 2024 20:09

Andarist reviewed Oct 15, 2024

View reviewed changes

VanTanev force-pushed the dependencies-replace-package-json branch from 2255082 to 9d2c046 Compare October 15, 2024 21:49

VanTanev changed the title ~~[dependencies-replacement] Replace package-json with a smaller implementation~~ [dependencies-replacement] Update package-json to latest, removing transitive dependencies Oct 16, 2024

VanTanev added 2 commits October 16, 2024 14:29

Upgrade package-json to latest version, setup babel to preserve dynam…

38ca596

…ic import() statements This removes 21 dependencies from @manypkg/cli

Fix fsevents install breaks on M1 macs

e5433c6

VanTanev force-pushed the dependencies-replace-package-json branch from f972808 to 2fce7e3 Compare October 16, 2024 11:30

Andarist reviewed Oct 17, 2024

View reviewed changes

VanTanev added 4 commits October 17, 2024 13:14

Bump required node version for @manypkg/cli to >= 18.0.0

b78a8e9

Upgrade sembear and semver

75ca2c7

changeset

d5cff5a

Upgrade to sembear@^0.7.0, removes one more dependency

f9fa19c

VanTanev force-pushed the dependencies-replace-package-json branch from 2fce7e3 to f9fa19c Compare October 17, 2024 10:14

Update changeset

4f534b0

Andarist merged commit aeb50f5 into Thinkmill:main Oct 17, 2024
4 checks passed

This was referenced Oct 17, 2024

Version Packages #227

Merged

Version Packages VanTanev/manypkg#1

Open

VanTanev mentioned this pull request Oct 17, 2024

Revert unnecessary change to babel config #234

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dependencies-replacement] Update package-json to latest, removing transitive dependencies #231

[dependencies-replacement] Update package-json to latest, removing transitive dependencies #231

VanTanev commented Oct 14, 2024 •

edited

Loading

changeset-bot bot commented Oct 14, 2024 •

edited

Loading

VanTanev commented Oct 14, 2024

Andarist commented Oct 14, 2024

Andarist Oct 14, 2024

VanTanev Oct 14, 2024 •

edited

Loading

Andarist Oct 14, 2024

VanTanev Oct 14, 2024

Andarist Oct 14, 2024

Andarist Oct 14, 2024

VanTanev Oct 14, 2024 •

edited

Loading

VanTanev Oct 15, 2024

Andarist Oct 15, 2024

VanTanev Oct 15, 2024

Andarist Oct 15, 2024

VanTanev Oct 15, 2024

Andarist Oct 15, 2024

Andarist Oct 17, 2024

Andarist Oct 15, 2024

Andarist Oct 15, 2024

VanTanev Oct 15, 2024 •

edited

Loading

Andarist Oct 15, 2024

VanTanev Oct 15, 2024

VanTanev Oct 16, 2024 •

edited

Loading

Andarist Oct 16, 2024

VanTanev Oct 16, 2024

emmatown Oct 16, 2024

VanTanev Oct 17, 2024

Andarist Oct 17, 2024

VanTanev Oct 17, 2024

VanTanev Oct 17, 2024

		// necessary to preserve dynamic import of ESM-only modules
		exclude: ["proposal-dynamic-import"],

[dependencies-replacement] Update package-json to latest, removing transitive dependencies #231

[dependencies-replacement] Update package-json to latest, removing transitive dependencies #231

Conversation

VanTanev commented Oct 14, 2024 • edited Loading

Additional changes

changeset-bot bot commented Oct 14, 2024 • edited Loading

🦋 Changeset detected

VanTanev commented Oct 14, 2024

Andarist commented Oct 14, 2024

Choose a reason for hiding this comment

VanTanev Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VanTanev Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VanTanev Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VanTanev Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VanTanev commented Oct 14, 2024 •

edited

Loading

changeset-bot bot commented Oct 14, 2024 •

edited

Loading

VanTanev Oct 14, 2024 •

edited

Loading

VanTanev Oct 14, 2024 •

edited

Loading

VanTanev Oct 15, 2024 •

edited

Loading

VanTanev Oct 16, 2024 •

edited

Loading