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

Merge exisiting package.json to package.json created by emberoider build #383

Merged
merged 2 commits into from
Feb 19, 2020

Conversation

SparshithNR
Copy link
Contributor

This uses the same logic from #178. I have reversed the order in which merging is done.
In #178 We merged new package.json content with old package.json content. That's why rebuild failed. If we just reverse the input values to the merge call it will fix the issue.

@ef4
Copy link
Contributor

ef4 commented Feb 10, 2020

This is still the wrong place to do this. It allows a data cycle. Each time the build runs, we read and write from the same file. So future builds get state mixed in from earlier builds.

This is the right location to write, but the reading of package.json needs to come earlier, before it gets written out into this location.

The package.json from fastboot is present in one of the inputPaths, and the AppDiffer is what's copying it into this directory.

@SparshithNR
Copy link
Contributor Author

SparshithNR commented Feb 13, 2020

but the reading of package.json needs to come earlier, before it gets written out into this location.

@ef4 I am not clear about this. Is there any recommendation where it can be done?

@ef4
Copy link
Contributor

ef4 commented Feb 13, 2020

This build method has inputPaths as an argument. For each path in inputPaths, look for a package.json file in that path. If it exists, load it. Merge together all the ones you find. Finally, merge on top of that this.app.packageJSON as before, and write out to pkgPath.

@SparshithNR
Copy link
Contributor Author

@ef4 I have updated the review with merging all package.json into one package.json file.

Copy link
Contributor

@ef4 ef4 left a comment

Choose a reason for hiding this comment

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

Thanks, this is closer now, but it's still reading from the output directory. It shouldn't be doing that.

Right now, the code

  1. Does merging of all addon-provided package.json in gatherAssets.
  2. Synthesizes a new assets from all of those which will get written to disk.
  3. Reads back in its own asset from the output directory, to merge more things into it and write it out again.

Instead, it should stop doing all the asset merging, and directly in build() it should call code that locates all package.jsonfiles ininputPaths`, merges them in memory, merges the app's own package.json (also still in memory), and writes out the file once.

@@ -680,6 +697,12 @@ export class AppBuilder<TreeNames> {
}
pkg['ember-addon'] = Object.assign({}, pkg['ember-addon'], meta);
const pkgPath = join(this.root, 'package.json');
if (existsSync(pkgPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still reading from the output directory.

1. Gather all the package.json from addons
2. Merge them together
3. Take app's package.json and merge with step2
4. Write it to the disk.
@ef4
Copy link
Contributor

ef4 commented Feb 18, 2020

^ That was just a rebase on master before I make a few small changes. Overall this is pretty close.

@ef4
Copy link
Contributor

ef4 commented Feb 18, 2020

Thanks. There were just a few small things needed:

  • using require for the package.json is likely to run afoul of node's require cache. Any rebuild changes that cause the package.json to change might not get picked up. So I used readJSONSync from fs-extra instead.
  • the assets:any type was too general, and was masking a bug. Assets are of multiple possible kinds, which needed to be handled.
  • I added regression test coverage.

@ef4 ef4 merged commit e8bee21 into embroider-build:master Feb 19, 2020
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.

None yet

2 participants