Skip to content
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

feat: recursively merge package.json scripts field from build configuration #1372

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented Aug 21, 2024

Issue #, if available:
closes #1125

Description of changes:

One of the build options for code generation is:

packageJson | required=No | Custom package.json properties that will be merged with the base package.json. The default value is an empty object.

Because of shallow merging, it is difficult for example to merge script entries like

{
  "scripts": {
    "build": "<from codegen>",
    "test": "<user provided>"
  }
}

This PR changes the behavior to recursive merging for only the scripts field. I do not expect anyone to be relying on the existing shallow merge behavior, because every code generated field in "scripts" is there for a reason.

More specific customization of the package.json object can be done after the build, if needed.

Recursive merging is not done for other fields, to avoid mixing user and generated nested fields in e.g. authorship metadata or bundler instruction metadata.

@kuhe kuhe requested review from a team as code owners August 21, 2024 16:05
@kuhe kuhe requested a review from gosar August 21, 2024 16:05
@kuhe kuhe force-pushed the feat/json-deep-merge branch 3 times, most recently from e309140 to 8682531 Compare August 21, 2024 16:19
@kuhe kuhe assigned kuhe and gosar Aug 22, 2024
.expectObjectNode();

ObjectNode mergedScripts = defaultPackageJson.expectObjectNode("scripts")
.merge(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the merge on line 57 not sufficient for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a shallow merge, so this does the following:

Default:

{
  "name": "A",
  "scripts": {
    "build": "..."  
  },
  "metadata": "A"
}

User supplied:

{
  "name": "B",
  "scripts": {
    "test": "..."  
  }
}
  • create the merged pkg.scripts object.
{
  "build": "...",
  "test": "..."
}
  • do the normal shallow merge
{
  "name": "B",
  "scripts": {
    "test": "..."  
  },
  "metadata": "A"
}
  • write the merged pkg.scripts onto the shallow merge
{
  "name": "B",
  "scripts": {
    "build": "...",
    "test": "..."  
  },
  "metadata": "A"
}

@kuhe
Copy link
Contributor Author

kuhe commented Aug 28, 2024

Aug 28: Need to address CI failures, I think they're related to this PR and not the intermittent kind

Oct 7: fixed

@gosar gosar removed their request for review October 2, 2024 18:39
@gosar gosar removed their assignment Oct 4, 2024
@kuhe kuhe merged commit 205cefb into smithy-lang:main Oct 7, 2024
11 checks passed
@kuhe kuhe deleted the feat/json-deep-merge branch October 7, 2024 17:32
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.

packageJson configuration is not deeply merged
4 participants