Merge pre-existing package.json with file generated by Cloverfield #40

halhenke · 2015-10-23T17:52:02Z

Re issue #23

Tried to make it fit the pre-existing code structure as much as possible.
There is some "intelligence" to the merging from the package-merge package rather than a straight object merge - keywords field will be merged without duplicates etc. Might want to take a quick look at that package to see if its desired behaviour:
- https://github.com/izaakschroeder/package-merge/blob/master/lib/merge.js

ericelliott · 2015-10-23T19:40:58Z

halhenke · 2015-10-23T19:56:51Z

Yeah, seems the guy who maintains package-merge didnt declare lodash as a dependency.

Its passing on my machine seemingly because its a dependency of precommit-hook thats been spat into node_modules by npm 3 as far as i can tell.

Not sure what the best approach is - theres not a ton of functionality in that package but i thought it would be nice to have. Could fork it and make a PR and in the meantime use the fork or do a simpler
{ ...origPkg, ...genPkg} type merge?

nkbt · 2015-10-24T07:27:01Z

package.json

@@ -54,6 +54,8 @@
    "husky": "^0.10.1",
    "isparta": "^3.0.4",
    "nsp": "^1.1.0",
+    "package-merge": "^0.1.2",
+    "precommit-hook": "^3.0.0",


oops, that was deleted previously

nkbt · 2015-11-25T09:46:56Z

Hi, @halhenke can you please update PR so I can merge it? thanks 👌

halhenke · 2015-11-25T12:42:49Z

OK for sure - sorry thought i already had - must be losing it... :-(

nkbt · 2015-11-27T10:05:22Z

package.json

@@ -57,6 +58,7 @@
    "husky": "^0.10.1",
    "isparta": "^3.0.4",
    "nsp": "^2.0.2",
+    "package-merge": "^0.1.2",


I guess this one is extra now.

Ah - i screwed up the rebase! will fix

nkbt · 2015-12-14T11:25:11Z

Sorry, for the delay, @halhenke
I've just checked the new generation process. Package.json is merged respecting existing values. Other files not, though. Like README.md has new values, though package.json still has old ones.

I am not sure about intended behaviour here. I can suggest that when we merge package.json we can see which values have been changed and replace them accordingly in other files too. But not touching values that should not be changed.

Alternative (and much easier) solution could be - read package.json to get "suggested" values for the second run. Then either I just hit "Enter" and use those default ones - or enter new ones - does not matter. Replace all things as usual.

Honestly after playing with it a bit I'd rather go for the second solution.

Related to #44 by the way.

ericelliott · 2015-12-14T22:44:51Z

👍

halhenke · 2015-12-18T12:39:32Z

Hey sorry for delay in getting back to you Nik - busy few days,

Didn't realise we wanted to merge README also - was only thinking of package.json.

I also like the second idea - at least it makes people aware of whats happening. Will rework it and see if i can come up with something that deals with other files - think there could be some hard to deal with edge cases - but otherwise will go for the user prompt option.

Did you want me to try and work in suggestions from #44 also (i got the impression the other guy didnt want to)? Dont mind.

nkbt · 2015-12-18T13:13:28Z

Awesome! #44 looks like duplicate to me then.

halhenke · 2015-12-18T13:35:32Z

Cool! 👍

ericelliott · 2016-02-08T20:11:52Z

What's going on with this?

halhenke · 2016-02-09T10:14:18Z

Sorry, sort of got lost in Christmas stuff - have a couple of free days, will try to finish it off.

ericelliott · 2016-02-10T00:09:08Z

Cool. Thanks!

…plate and tests

…nal package fields over template ones

…rom pre-existing package.json if available

…s into template package.json

- change big chain of if else statements into something a little more extensible - move author/github stuff into separate file

halhenke · 2016-02-18T11:29:50Z

OK this thing has been dragging on for a while (apologies) would love to wrap it up

Not entirely happy with what i've done (it feels a bit hack-ish) but wanted to check its the direction we wanted.

Anyway if a package.json file is found that exists:

grab author/email/github from package.json & use as defaults
read dependecies, devDependencies and scripts fields and add questions whether the user wants to merge them into the template package.json (could easily add other fields like peerDependencies if you like)

Feels a bit inelegant because its not using the same handlebars template/vars method to modify most fields - though we do now have author/email variables etc that will now be used in other templates also.

Could use more tests. Anyway just wanted to get any feedback/suggested changes when you guys have time.

halhenke · 2016-04-02T10:36:39Z

Hey guys - just wanted to say - if this PR just missed the mark or isnt working out I wont be offended if you just want to kill it or anything. Its all pretty fuzzy for me at this point anyway.

ericelliott · 2016-04-03T18:20:56Z

@nkbt Comments?

nkbt · 2016-04-05T10:43:27Z

Wow, that looks cool, pretty much package.json-specific merging. I wish github sends emails on PR updated. I'll try it on couple of existing projects and let you know asap!

ericelliott · 2016-04-13T21:50:07Z

@nkbt Any more thoughts?

nkbt · 2016-05-18T09:05:46Z

lib/mergePack.js

+    const newPackagePath = path.join(process.cwd(), 'package.json');
+    const packageIndex = destinations.indexOf(newPackagePath);
+    const packageContents = JSON.parse(compiledFiles[packageIndex]);
+    const mergedPack = JSON.stringify(merge(packageContents, origPackage), null, 2);


I've tried it several times and tried to fix it too... No luck. With current implmentation no matter what I chose deps/devdeps/scrips are preserved and never updated. If I swap packageContents and origPackage - they always updated instead. Somehow those props from setPackageProps are just totally ignored =(

halhenke · 2016-05-19T02:51:31Z

😞

I'll take a look at it either tonight or later this weekend.

nkbt reviewed Oct 24, 2015
View reviewed changes

halhenke force-pushed the feaure/package-check branch from 6171927 to c1d4b76 Compare November 26, 2015 03:00

nkbt reviewed Nov 27, 2015
View reviewed changes

halhenke added 9 commits February 10, 2016 22:26

require package-merge

d4771ad

functions to merge a pre-existing package.son file with generated tem…

5c736c9

…plate and tests

Add merge package step between template compilation and content saving

d753600

export functions rather than object in mergePack

2a626cc

Swap out package-merge for loads.merge for package.json merging

0746fa5

test that package.json output has decent formatting

4bb6fb4

Get rid of unused package.merge package

81f1663

Swap order of arguments in loads.merge in order to choose users origi…

4aa5791

…nal package fields over template ones

Add object rest/spread syntax

763e1af

halhenke force-pushed the feaure/package-check branch from 334399b to 5907619 Compare February 10, 2016 17:35

halhenke force-pushed the feaure/package-check branch from 634d583 to 91b327e Compare February 18, 2016 11:04

halhenke added 4 commits February 18, 2016 22:13

Very Rough version of changes

3a4f07a

use parse-author & parse-github-url to get author & Github info f…

5bb8e17

…rom pre-existing package.json if available

Option to merge pre-existing dependencies, devDependencies and script…

959e47f

…s into template package.json

Refactor

52e69f8

- change big chain of if else statements into something a little more extensible - move author/github stuff into separate file

halhenke force-pushed the feaure/package-check branch from 91b327e to 52e69f8 Compare February 18, 2016 11:18

nkbt reviewed May 18, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge pre-existing package.json with file generated by Cloverfield #40

Merge pre-existing package.json with file generated by Cloverfield #40

halhenke commented Oct 23, 2015

ericelliott commented Oct 23, 2015

halhenke commented Oct 23, 2015

nkbt Oct 24, 2015

nkbt commented Nov 25, 2015

halhenke commented Nov 25, 2015

nkbt Nov 27, 2015

halhenke Nov 27, 2015

nkbt commented Dec 14, 2015

ericelliott commented Dec 14, 2015

halhenke commented Dec 18, 2015

nkbt commented Dec 18, 2015

halhenke commented Dec 18, 2015

ericelliott commented Feb 8, 2016

halhenke commented Feb 9, 2016

ericelliott commented Feb 10, 2016

halhenke commented Feb 18, 2016

halhenke commented Apr 2, 2016

ericelliott commented Apr 3, 2016

nkbt commented Apr 5, 2016

ericelliott commented Apr 13, 2016

nkbt May 18, 2016

halhenke commented May 19, 2016 •

edited

Merge pre-existing package.json with file generated by Cloverfield #40

Are you sure you want to change the base?

Merge pre-existing package.json with file generated by Cloverfield #40

Conversation

halhenke commented Oct 23, 2015

ericelliott commented Oct 23, 2015

halhenke commented Oct 23, 2015

nkbt Oct 24, 2015

Choose a reason for hiding this comment

nkbt commented Nov 25, 2015

halhenke commented Nov 25, 2015

nkbt Nov 27, 2015

Choose a reason for hiding this comment

halhenke Nov 27, 2015

Choose a reason for hiding this comment

nkbt commented Dec 14, 2015

ericelliott commented Dec 14, 2015

halhenke commented Dec 18, 2015

nkbt commented Dec 18, 2015

halhenke commented Dec 18, 2015

ericelliott commented Feb 8, 2016

halhenke commented Feb 9, 2016

ericelliott commented Feb 10, 2016

halhenke commented Feb 18, 2016

halhenke commented Apr 2, 2016

ericelliott commented Apr 3, 2016

nkbt commented Apr 5, 2016

ericelliott commented Apr 13, 2016

nkbt May 18, 2016

Choose a reason for hiding this comment

halhenke commented May 19, 2016 • edited

halhenke commented May 19, 2016 •

edited