Beta: ember g ember-electron rewrites package.json without asking for diff/user interaction

[pichfl]

While it should do no harm (we are in a git repo after all), the current blueprint may lead to data loss in case a user has data in the package.json "config" property that does not belong to ember-electron.

[bendemboski]

It shouldn't cause any data loss unless the user had a previous forge configuration -- see here.

[pichfl]

I still wonder if we should add a quick confirm message there or maybe just explain whats happening while it happens in the command line.

[weedgrease]

i had my forge configuration there and sure, while it's all saved in git, it was confusing/annoying that other files had me confirm, but not package.json.

[bendemboski]

Perhaps -- I wasn't arguing that we shouldn't, just pointing out that the severity is less than originally described because it's pretty unlikely the user would experience any actual data loss.

On the one hand, it is modifying a file, and on the other hand, blueprints modify package.json all the time without confirmation from the user, although in those cases it's just adding dependencies.

Personally, I'd opt for one of these options:

1. Don't prompt the user because adding manual steps to everybody's install/blueprint process for what I believe is an edge case seems wrong.
2. Look to see if there's already a forge config there that is different from what we are about to apply, and if so, prompt the user at that point (this essentially matches the default blueprint behavior of doing nothing when files are identical, not prompting when the file doesn't already exist, and only prompting if something is going to be overwritten).

[anulman]

Now that we're back to using forge import:

• They edit package.json already upstream without warning the user; it is non-destructive insofar as it will not overwrite config or config.forge if they exist;
• We are rewriting the provided forge config to a file

I'm going to update #188 to ensure the forge config is an object, vs. a string. This satisfies Ben's first point, and is non-destructive to downstream use:

• If config.forge was an object, forge import does not overwrite it & we write it out to a file instead;
• If config.forge was a string, forge import does not overwrite it & we ignore it.

IMO let's close this; it's now noted & searchable for downstream users + we can reopen if anyone actually has a problem.

[bendemboski]

I'd prefer to find a way to prompt after thinking about this further. My original comment was only thinking of running the blueprint at first install. One of the basic behaviors of Ember blueprints is that you can re-run them to update to the latest version of the blueprint, and it will prompt you for any overwrites. I think we should maintain that here (which will probably involve upstream changes to electron-forge) otherwise anytime something in the blueprint changes and users update and re-runs the blueprint it will overwrite their forge config without prompting, which is a violation of how Ember blueprints are supposed to behave.

[anulman]

If that's the case and someone feels passionate about pursuing this (i.e. not me, at least until it's proven to be an issue for non-maintainer consumers), let's create an issue upstream and see what happens.

Until then, I strongly suggest we close this issue, as:

• Today's behaviour is non-destructive;
• There are more valuable issues & features to focus on IMO; and
• There's no way to implement this behaviour in e-electron until it's fixed upstream.

[bendemboski]

I disagree with your first point -- it will be destructive when users re-run the blueprint to pick up changes in future ember-electron releases.

I think there's a middle ground that doesn't require us to wait for electron-forge to support our use case. There are two ways this could be destructive:

1. The user has their own config inline in package.json which I think you just took care of in your branch?
2. The user has a customized config in their .electron-forge.js, which I think is highly likely.

In (2), you could just check the contents of .electron-forge.js to see if it matches what you're about to write, and if it differs, prompt the user before overwriting. I haven't looked, but I would hope that the prompting/diffing is in a form that we can use directly from ember-cli's blueprint code.

[bendemboski]

Or, option (3) is to go back to keeping the default config in a file in the blueprint so it automatically does the ember blueprint goodness of prompting before overwrite, and then just modify your code to only re-write the config in package.json, and not copy the contents into .electron-forge.js because that's handled by the default blueprint code.

The only maintenance overhead we'd be embracing would be to update the blueprint file anytime electron-forge changes their default config. But those changes would have to either be backwards-compatible, or be in a new major version or else we'd be boned anyway because the ^xx semver version would allow users to get updated versions of electron-forge without knowing it, and without re-running the blueprint to update the config. So really we'd only need to update the config when we accept a breaking-change update of electron-forge into ember-electron.

[anulman]

1. The op is / will be non-destructive in our lib (see 188)
2. The op is destructive in forge atm (see api/import), but clearly isn't intended to be. I have asked about this as well in Slack, and will put up this PR myself if need be. That "false" default makes sense for them too.

Overall, @bendemboski per our Slack chats, I think you are being too eager to take on technical debt vs. affect upstream projects.

[anulman]

Looping back, @malept confirmed that overwriting things on re-running api/import is not the intent; will start tracking this upstream soon.

[bendemboski]

Will we be able to diff/prompt for overwrite? Or are we abandoning the standard Ember blueprint behavior?

[anulman]

How about forgoing package.json changes entirely and handling in broccoli?

[bendemboski]

Yeah, regardless of this discussion that's a great idea!

It doesn't address my blueprint concern, though. From the user's perspective we're doing a very normal Ember blueprint thing -- generating a file that the user can customize. So when they re-run the blueprint, I think they'll be expecting the very normal Ember blueprint behavior -- prompt to replace/ignore/view diff.

[anulman]

Fair enough. Will change #188 to account for at least handling package.json changes. I'll also take a look into e-cli's guts and figure out how best to handle the prompt, given we're not diffing against files.

If the prompt / diff work proves substantive, I'd prefer to land #188 ASAP, keep this issue open, and handle it "soon" (i.e. late-2.0, or in a 2.0.1). The PR does + will not risk destructive behaviour, and I believe landing the test lib and transitioning the community to a stable 2.0 are higher-value tasks.

[bendemboski]

Sounds like a good plan for changing/investigating. If the prompt/diff work proves substantive, I would argue for just throwing the file into the blueprint for now until we have a chance to investigate. If we're planning to put off some work for later either way, then let's opt for the short-term solution that doesn't compromise the user experience.

[pichfl]

I think the solution that went into 2.0 is fine with me so I consider this resolved.