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

Allow for package.json deep merge to support more scenarios #1058

Open
gossi opened this issue Jan 26, 2025 · 5 comments
Open

Allow for package.json deep merge to support more scenarios #1058

gossi opened this issue Jan 26, 2025 · 5 comments

Comments

@gossi
Copy link

gossi commented Jan 26, 2025

When supporting addons from ember@3 up to ember@6, there needs to happen various changes here and there.

My current problem is roughly mentioned here: embroider-build/addon-blueprint#131
Basically the resolving @ember/owner in ember <> 4.12 - that in some scenarios is missresolved by the package manager, namely pnpm for me. That's why we are advised to use dependenciesMeta.*.injected. However, that comes with a big downside for actually developing the addon and requires third-party-tools only to fix that.

A handy trick would be for CI to have that scenario modelled, in my case for older ember versions.
Here is my sample commit for that: gossi/ember-ability@9ecfd9b which adds a script and dependenciesMeta to some package.json - but that does not work, because only a couple of selected properties are overwritable, see:

_packageJSONForDependencySet(packageJSON, dependencySet) {
this._overridePackageJSONDependencies(packageJSON, dependencySet, 'dependencies');
this._overridePackageJSONDependencies(packageJSON, dependencySet, 'devDependencies');
this._overridePackageJSONDependencies(packageJSON, dependencySet, 'peerDependencies');
this._overridePackageJSONDependencies(packageJSON, dependencySet, 'ember');
this._overridePackageJSONDependencies(packageJSON, dependencySet, this.overridesKey);
return packageJSON;
}

What about a deep merge instead and everybody is free to change what they seem necessary for them?

@kategengler
Copy link
Member

I think deep merge might be a bit too free, but I support adding hooks to do this kind of thing.

I ran into the issue with dependenciesMeta*injected this week. I found on pnpm v10 it wasn't necessary but also that removing it exposed issues with ember-try with pnpm in workspaces that causes errors around peer deps. I think we can solve this in ember-try.

@gossi
Copy link
Author

gossi commented Jan 27, 2025

I forgot to share my thoughts behind this, let me add them.

I was thinking a deep merge might be a bit too much in the first place, too. I had some security gotchas in mind, but I don't think they hold true as its the engineers who write these scenarios. If there really is something I'm not seeing it - please let us know.

Then I thought, ok allow more fields to be overwritten. But then it comes down to which fields and in essence, that ember-try will make a judgement what fields engineers are allowed to overwrite. I consider its the engigneers who best know what they need to change, which is why I considered this judgement a bit too limited and will result in "one issue/pr for a each new field"

I ended with the idea for a deep merge, as it is in control of package authors, neither harmful nor limiting in any form but enabling in any way possible.

How does that incorporate with your thoughts?

@kategengler
Copy link
Member

I could be convinced of a deep merge. I have also long thought a hook that is passed the package.json and allows users to do what they will would good.

I'd like to hear @bertdeblock's thoughts since he's been doing the most maintenance of this package.

@bertdeblock
Copy link
Member

My simplified ember-try clone (try-or-die) uses a "deep" merge/set approach. It doesn't deep-merge arrays though, but it is super flexible.

It will work well for single-project repos, but in case of pnpm workspaces, e.g. overrides need to be defined in the root package.json file. So that needs to be taken into account as well somehow.

If we would do something like this, packageJson might be better as a config key than npm as well?

Also, with the current API, changing package manager is a single line change, which would not be the case with the deep-merge strategy (different keys/locations for overrides/resolutions). Probably not a big issue, but mentioning it nonetheless.

@gossi
Copy link
Author

gossi commented Jan 28, 2025

I could be convinced of a deep merge. I have also long thought a hook that is passed the package.json and allows users to do what they will would good.

I like that idea. Usually other configs look like: "bring us the config as object or use a function to have full control over this". Putting this into some types with what bert said:

interface EmberTryConfig {
  packageJson: object | (packageJson: object) => object;
  rootPackageJson: object | (packageJson: object) => object;
  // ...
}

something around these lines?

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

No branches or pull requests

3 participants