Skip to content

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

Merged

Conversation

simschla
Copy link
Contributor

@simschla simschla commented Jul 3, 2025

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 new serve.js files (containing the UUID-change) because the md5 hash used in the node-modules-dir's name did only respect the package.json content, not the serve.js content.

This PR changes this, so that the content of the package.json as well as the content of the serve.js files is respected in order to calculate a md5 hash for naming the node-modules-dir. So if any of these files change, a new node-modules-dir is used.

This PR also bumps the dependencies used behind the scenes for creating and managing the npm server process.

simschla added 5 commits July 3, 2025 20:22
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.
@simschla simschla requested a review from Copilot July 3, 2025 19:04
Copy link
Contributor

@Copilot Copilot AI left a 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 and serve.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 to additionalFileContentStream 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 and http-graceful-shutdown are declared under devDependencies. If these scripts run in production, consider moving them to dependencies 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 and http-graceful-shutdown are declared under devDependencies. If these scripts run in production, consider moving them to dependencies 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 and http-graceful-shutdown are declared under devDependencies. If these scripts run in production, consider moving them to dependencies to ensure they're always installed.
		"express": "5.1.0",

@nedtwigg nedtwigg merged commit e803400 into diffplug:main Jul 7, 2025
30 of 31 checks passed
@nedtwigg
Copy link
Member

nedtwigg commented Jul 7, 2025

Released in plugin-gradle 7.1.0 and plugin-maven 2.45.0.

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