-
Notifications
You must be signed in to change notification settings - Fork 481
fix: bump npm infrastructure dependencies and make sure changing serve.js content is applied #2542
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
fix: bump npm infrastructure dependencies and make sure changing serve.js content is applied #2542
Conversation
this was a hidden update issue: if we change the serve-script content between two releases but the user keeps the package.json the same, the md5 hash used for the node-modules-dir stayed the same, resulting in us not using the latest version of the serve script but keep using the old one. That was especially harmful in the case where we changed the api of the serve-script to accept a uuid to allow multiple instances. The old script just ignored that resulting in a server-process that was never found due to wrong port-file-names written/waited for.
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.
Pull Request Overview
This PR ensures the serve.js content is included when hashing node-modules
directories and bumps the underlying npm server dependencies.
- Extend MD5 hashing to cover both
package.json
andserve.js
contents - Update NodeServerLayout and step state to pass through serve script content
- Bump versions of Express and graceful-shutdown packages in all serve scripts
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
lib/src/test/java/com/diffplug/spotless/npm/NpmResourceHelperTest.java | Added unit tests for the new MD5 overload |
lib/src/test/java/com/diffplug/spotless/npm/NodeServerLayoutTest.java | Added tests to verify the hash changes when serve.js differs |
lib/src/main/resources/com/diffplug/spotless/npm/tsfmt-serve.js | Fixed typo when sending error messages |
lib/src/main/resources/com/diffplug/spotless/npm/common-serve.js | Replaced old shutdown manager with http-graceful-shutdown |
lib/src/main/resources/com/diffplug/spotless/npm/tsfmt-package.json | Bumped server package to v4.0.0 and updated dependencies |
lib/src/main/resources/com/diffplug/spotless/npm/prettier-package.json | Bumped server package to v4.0.0 and updated dependencies |
lib/src/main/resources/com/diffplug/spotless/npm/eslint-package.json | Bumped server package to v4.0.0 and updated dependencies |
lib/src/main/java/com/diffplug/spotless/npm/NpmResourceHelper.java | Extended md5 to accept multiple contents and updated implementation |
lib/src/main/java/com/diffplug/spotless/npm/NpmFormatterStepStateBase.java | Pass serve.js content into NodeServerLayout |
lib/src/main/java/com/diffplug/spotless/npm/NodeServerLayout.java | Revised directory name generation to include serve script hash |
lib/build.gradle | Added testlib project to testing classpath |
Comments suppressed due to low confidence (5)
lib/src/main/java/com/diffplug/spotless/npm/NpmResourceHelper.java:146
- [nitpick] The variable name
additionalFilecontentStream
mixes casing and violates CamelCase conventions. Consider renaming it toadditionalFileContentStream
for consistency.
Stream<String> additionalFilecontentStream = Stream.concat(
lib/src/main/resources/com/diffplug/spotless/npm/common-serve.js:3
- [nitpick] The imported function is assigned to a generic name. Consider renaming the shutdown handler (e.g., to
shutdownServer
) to avoid confusion with system-level shutdown methods.
const gracefulShutdown = require("http-graceful-shutdown");
lib/src/main/resources/com/diffplug/spotless/npm/tsfmt-package.json:12
- Runtime dependencies like
express
andhttp-graceful-shutdown
are declared underdevDependencies
. If these scripts run in production, consider moving them todependencies
to ensure they're always installed.
"express": "5.1.0",
lib/src/main/resources/com/diffplug/spotless/npm/prettier-package.json:12
- Runtime dependencies like
express
andhttp-graceful-shutdown
are declared underdevDependencies
. If these scripts run in production, consider moving them todependencies
to ensure they're always installed.
"express": "5.1.0",
lib/src/main/resources/com/diffplug/spotless/npm/eslint-package.json:12
- Runtime dependencies like
express
andhttp-graceful-shutdown
are declared underdevDependencies
. If these scripts run in production, consider moving them todependencies
to ensure they're always installed.
"express": "5.1.0",
lib/src/main/java/com/diffplug/spotless/npm/NpmResourceHelper.java
Outdated
Show resolved
Hide resolved
lib/src/main/resources/com/diffplug/spotless/npm/common-serve.js
Outdated
Show resolved
Hide resolved
Released in |
In #2462 we introduced the support for using the same
node-modules
-dir for multiple spotless steps requiring the same dependencies. To achieve this, the serve.js files were updated to accept a UUID when starting a npm-based server so that the java side knows which server-process (which port) it may use.This created a problem we did not foresee: If a user has a running spotless installation before this change, then updates to the version containing this change, it leads to a problematic situation: the existing
node-modules
-dir (created by the previous spotless version and thus containing the old serve.js files without the UUID-change) would prevent the new spotless version from using the newserve.js
files (containing the UUID-change) because the md5 hash used in thenode-modules
-dir's name did only respect the package.json content, not theserve.js
content.This PR changes this, so that the content of the
package.json
as well as the content of theserve.js
files is respected in order to calculate a md5 hash for naming thenode-modules
-dir. So if any of these files change, a newnode-modules
-dir is used.This PR also bumps the dependencies used behind the scenes for creating and managing the npm server process.