apache
[WIP] package.json helper class
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 Report
Merging #34 into master will increase coverage by
0.3%. The diff coverage is98.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 dataPowered by Codecov. Last update 2110597...5c32096. Read the comment docs.
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.
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:
- [x] Modify package.json while keeping its formatting intact
- [x] Safely get the cordova object
- [x] Safely get the list of plugins/platforms
- [x] 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.
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.
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.
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,specandvariablekeys 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.
TODO: Address https://github.com/apache/cordova-lib/issues/694 here to avoid writing if there are no changes
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 [email protected] 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 https://github.com/apache/cordova-common/pull/34#issuecomment-423641044, or mute the thread https://github.com/notifications/unsubscribe-auth/ABfNUDkzo3kEjDwvscmbOavC1rQQd3dNks5udTjSgaJpZM4VSRfM .
I'm hoping to update this before it's merged. This probably won't make the next major
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,specandvariablekeys 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-createcould easily be satisfied by a generic cordova-unawarePackageFileEditor, for example. Sincenameandversionkeys 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 hypotheticalCordovaPackageFileEditorcould delegate to thePackageFileEditorfor file reading and writing and focus on accessing/updating the contents of thecordovakey.
Edit: Please note that this is just me trying to give feedback to make a good thing even better. No offense intended.
@raphinesse Thanks for the re-review! I agree with almost all of those comments. Funny thing, when working on https://github.com/apache/cordova-lib/pull/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,specand variable keys describes a plugin).
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.
https://github.com/apache/cordova-common/blob/f9951b360f8db344f919b1ee0c87c26661698a98/src/ConfigParser/ConfigParser.js#L400-L427
https://github.com/apache/cordova-common/blob/f9951b360f8db344f919b1ee0c87c26661698a98/src/ConfigParser/ConfigParser.js#L355-L359
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
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 :)
