[WIP] package.json helper class

[dpogue]

Platforms affected

Tooling

What does this PR do?

Adds a class to manage finding and updating the cordova section of package.json, while also being consistent with npm about how newlines and indentation are handled.

The idea is to use this class in refactoring some pieces of cordova-lib.

I also added JSDoc/TypeScript-style doc comments to the class, which will provide autocompletion and tooltip information for anyone using VS Code. This partially helps to more clearly define what is "public"/private API, and also provides the opportunity to do type checking as a future testing step.

What testing has been done on this change?

Unit tests written with 100% coverage.

[codecov-io]

Codecov Report

Merging #34 into master will increase coverage by 0.3%.
The diff coverage is 98.33%.

@@            Coverage Diff            @@
##           master      #34     +/-   ##
=========================================
+ Coverage   88.85%   89.16%   +0.3%     
=========================================
  Files          19       20      +1     
  Lines        1795     1855     +60     
  Branches      368      385     +17     
=========================================
+ Hits         1595     1654     +59     
- Misses        200      201      +1

Impacted Files	Coverage Δ
src/PackageHelper.js	98.33% <98.33%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2110597...5c32096. Read the comment docs.

[brodycj]

I also added JSDoc/TypeScript-style doc comments to the class

+1

[dpogue]

If we add support for changing packaging name and version, then yes, I'd agree. Currently though, it's intended that addPlatform/addPlugin are only called when something is actually being added for the first time to the project.

[raphinesse]

I think I would make writes explicit either way. Reasons:

• to me that would be what I would expect
• User can decide whether they want async or sync writing
• Better support for batch updates (3 new plugins, 3 writes = nasty)

I've had a few more thoughts about this:

My first thought was that this actually solves more problems than it should. To me, modifying a package.json file while keeping its formatting intact and updating cordova specific contents of package.json seem to be different enough to warrant separate tools (Interface segregation principle). I think the very generic name is partly a symptom of that too.

The use cases for cordova-create could easily be satisfied by a generic cordova-unaware PackageFileEditor, for example. Since name and version keys live on the top level, I need nothing to navigate the package object and indeed would prefer to keep it simple and just have access to it as a plain object. A hypothetical CordovaPackageFileEditor could delegate to the PackageFileEditor for file reading and writing and focus on accessing/updating the contents of the cordova key.

Trying to formulate suggestions for other aspects of this class, I found that it was very hard to do so because I couldn't exactly pinpoint the scope of this class.

Some use cases that I saw covered by this class:

• Modify package.json while keeping its formatting intact
• Safely get the cordova object
• Safely get the list of plugins/platforms
• Get version information for plugins and platforms

Use cases I came up with but that weren't handled (they might be irrelevant; I just don't know):

• Safely get plugin variables
• Make any other modifications to the cordova object
• Easily get the spec of a specific platform (i.e regardless of a cordova- prefix)

Maybe you have a better overview of the relevance of these use cases in the current code base.

Another thought was, that we might even want to aggregate information for plugins/platforms like it is in config.xml (i.e. an object with name, spec and variable keys describes a plugin). Even more so, we might actually want to have an interface that consistently manages platforms and plugins in package.json AND config.xml so the programmers do not have to keep them in sync themselves.

I hope these remarks weren't too much of a stream of consciousness. I just wanted to get out some of my thoughts about this in the hopes that some of them might actually be helpful. I find it very hard to do the right thing here, which is probably at least in part due to the fact that this change intersects with the topic of package management in Cordova and the messed up state of the latter.

[raphinesse]

I think this getter and the related platform and plugin getters are somewhat problematic. Consider the following scenario:

const ph = new PackageHelper(...);
ph.cordova.foo = 'bar';
ph.writeSync();

I guess that's not a use case you had in mind, but it's possible. The problem is it persists foo if and only if we already had a cordova object. A more probable scenario with a similar problem would be a batch update of platforms:

const ph = new PackageHelper(...);
ph.cordovaPlatforms.push(...['amiga', 'c64', 'famicon']);
ph.writeSync();

[dpogue]

Maybe you have a better overview of the relevance of these use cases in the current code base.

One problem is that this class was designed as the first step in a larger proposal that I never finished writing/formatting for review. I'll try my best to get that posted on cordova-discuss today. 😅

I think this getter and the related platform and plugin getters are somewhat problematic.

Maybe those getters should return readonly/frozen copies of the data, to prevent trying to assign into them. They were not intended to be used for writing.

[raphinesse]

Maybe those getters should return readonly/frozen copies of the data, to prevent trying to assign into them.

That's exactly what I was thinking. We would need a deepFreeze lib though.

[dpogue]

I've updated this to return frozen data anywhere that it's not returning a simple type like a string. I've also added getPluginVariables(pluginName) to safely get plugin variables, as well as getters for packageID (the name field) and version.

I've removed the calls to write, so consumers of this API will need to remember to call write() or writeSync() after making any changes.

we might even want to aggregate information for plugins/platforms like it is in config.xml (i.e. an object with name, spec and variable keys describes a plugin). Even more so, we might actually want to have an interface that consistently manages platforms and plugins in package.json AND config.xml so the programmers do not have to keep them in sync themselves.

The plan is to deprecate storing any of that information in config.xml and keep it all in package.json, which this class is intended to help with. Right now it's a giant mess of over-complicated code that tries to keep them in sync, and seems to pick which file overrules the other at random, which was done as a sort of transition period. We want to only look at package.json for dependencies going forward and keep config.xml reserved for application config stuff like preferences, icons, and splashscreens.

[raphinesse]

Looks very good to me. Left one little remark in the code.

Another thing I noticed, was that there are no methods for removing platforms and plugins. Do you plan adding some?

That being said, I think this would already be super useful as it is.

[raphinesse]

I would have expected this to replace the variables, not merge them. With the current implementation we cannot remove variables. Merging would still be doable, if a little verbose:

const oldVars = pkgHelper.getPluginVariables('cordova-plugin-foo')
pkgHelper.addPlugin('cordova-plugin-foo', Object.assign({}, oldVars, newVars))

I couldn't tell whether we have to merge often. If so, we should probably provide an option for that.

[dpogue]

TODO: Address apache/cordova-lib#694 here to avoid writing if there are no changes

[dpogue]

Also possibly worth looking at: https://github.com/npm/read-package-json

[brodycj]

I hope the Todo and read-package-json ideas will be kept in a new issue to avoid losing them after the merge.

…

On Fri, Sep 21, 2018, 3:06 PM Darryl Pogue ***@***.***> wrote: Also possibly worth looking at: https://github.com/npm/read-package-json — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#34 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABfNUDkzo3kEjDwvscmbOavC1rQQd3dNks5udTjSgaJpZM4VSRfM> .

[dpogue]

I'm hoping to update this before it's merged. This probably won't make the next major

[brodycj]

I just marked this as [WIP] to avoid a premature merge.

[raphinesse]

modified should be reset after writing out to disk.

I'd personally prefer to keep a copy of the initial contents and use deepEqual to check if need to write to disk. That would also catch use-cases where we need to manipulate pgkfile directly.

---

cordovaDependencies should be updated to avoid problems when dependencies or devDependencies are undefined:

get cordovaDependencies () {
    const mergedDependencies = Object.assign({}, this.pkgfile.dependencies, this.pkgfile.devDependencies);
    return deepFreeze(
        this.cordovaPlatforms
            .map(p => p.startsWith('cordova-') ? p : `cordova-${p}`)
            .concat(this.cordovaPlugins)
            .reduce((deps, cur) => {
                deps[cur] = mergedDependencies[cur];
                return deps;
            }, {})
    );
}

---

we might even want to aggregate information for plugins/platforms like it is in config.xml (i.e. an object with name, spec and variable keys describes a plugin).

When test-driving this class on the new restoreUtil.installPlatformsFromConfigXML aggregate plugin information as requested above would still have been useful, even when we are not using config.xml to store dependency information anymore.

---

As this class has reached quite some size and still could do more (see block above) I stand by my remarks regarding interface segregation from above and would appreciate feedback on it:

My first thought was that this actually solves more problems than it should. To me, modifying a package.json file while keeping its formatting intact and updating cordova specific contents of package.json seem to be different enough to warrant separate tools (Interface segregation principle). I think the very generic name is partly a symptom of that too.

The use cases for cordova-create could easily be satisfied by a generic cordova-unaware PackageFileEditor, for example. Since name and version keys live on the top level, I need nothing to navigate the package object and indeed would prefer to keep it simple and just have access to it as a plain object. A hypothetical CordovaPackageFileEditor could delegate to the PackageFileEditor for file reading and writing and focus on accessing/updating the contents of the cordova key.

---

Edit: Please note that this is just me trying to give feedback to make a good thing even better. No offense intended.

[dpogue]

@raphinesse Thanks for the re-review! I agree with almost all of those comments. Funny thing, when working on apache/cordova-lib#752 I was referencing back to this PR and actually wrote it to use this class's API before refactoring it to the expanded form.

I definitely see the value in splitting this up into a PackageFileEditor and a CordovaPackageFileEditor subclass. 🙂

The only suggestion I'm not really a fan of is changing the format of the data stored in package.json, unless you were thinking just of providing a getter that returned an object with aggregated data?

we might even want to aggregate information for plugins/platforms like it is in config.xml (i.e. an object with name, spec and variable keys describes a plugin).

[raphinesse]

The only suggestion I'm not really a fan of is changing the format of the data stored in package.json, unless you were thinking just of providing a getter that returned an object with aggregated data?

Yes, my suggestion was to provide getters similar to ConfigParser.getPlugin and maybe ConfigParser.getPlugins while keeping the data format stored in package.json untouched.

cordova-common/src/ConfigParser/ConfigParser.js

Lines 400 to 427 in f9951b3

	getPlugin: function (id) {
	if (!id) {
	return undefined;
	}
	var pluginElement = this.doc.find('./plugin/[@name="' + id + '"]');
	if (pluginElement === null) {
	var legacyFeature = this.doc.find('./feature/param[@name="id"][@value="' + id + '"]/..');
	if (legacyFeature) {
	events.emit('log', 'Found deprecated feature entry for ' + id + ' in config.xml.');
	return featureToPlugin(legacyFeature);
	}
	return undefined;
	}
	var plugin = {};
	plugin.name = pluginElement.attrib.name;
	plugin.spec = pluginElement.attrib.spec || pluginElement.attrib.src || pluginElement.attrib.version;
	plugin.variables = {};
	var variableElements = pluginElement.findall('variable');
	variableElements.forEach(function (varElement) {
	var name = varElement.attrib.name;
	var value = varElement.attrib.value;
	if (name) {
	plugin.variables[name] = value;
	}
	});
	return plugin;
	},

cordova-common/src/ConfigParser/ConfigParser.js

Lines 355 to 359 in f9951b3

	getPlugins: function () {
	return this.getPluginIdList().map(function (pluginId) {
	return this.getPlugin(pluginId);
	}, this);
	},

[dpogue]

Okay cool, that definitely sounds reasonable to do :)

[piotr-cz]

First of all sorry for my ignorace, as I haven't read this PR's commits and comments.

I just want to say that IMHO package.json should be saved in same format as npm uses.
At the moment every npm operation adds newline at the end of the file and every cordova operation removes it.

---

Update

Sorry, didn't notice that sentences in PR description: consistent with npm about how newlines and indentation are handled

BTW it's handled via stringify-package package

[brodycj]

What is the status of this?

[dpogue]

What is the status of this?

No change in status. I've had no time to look at this any further.
I think the next step was going to be splitting this into two classes: one that handles package.json, and one that is a subclass that handles the cordova-specific bits.

BTW it's handled via stringify-package package

Yep, I extracted that from npm CLI into its own module as part of putting this proposal together :)

[breautek]

this package is deprecated now, the replacement is https://www.npmjs.com/package/@npmcli/package-json

[erisu]

I think we can actually close this PR for @npmcli/package-json.