You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by akdor1154 <gi...@git.apache.org> on 2017/10/24 04:58:37 UTC
[GitHub] cordova-lib pull request #602: CB-12774 : Don't munge scoped plugin IDs anym...
GitHub user akdor1154 opened a pull request:
https://github.com/apache/cordova-lib/pull/602
CB-12774 : Don't munge scoped plugin IDs anymore. Comment requested.
In order to get plugins that are under an npm scope (e.g. `@akdor1154/some-plugin`) to install, a previous change from took the approach to consider such a plugin to be called (have the id) `some-plugin` instead of `@akdor1154/some-plugin`. This allowed scoped plugins to install and preserved the assumption that plugins will always be installed in `plugins_dir/[plugin_id]`.
However, it required special parsing logic around `npm` package IDs, and it broke the assumption that the `name` in an npm plugin's `package.json' would correspond to Cordova's idea of a plugin ID. IMO this approach is the source of further complexity which is not required, and is leading to weird bugs and special cases with scoped plugins. (see the linked issue, but there is stuff as basic as "`npm install` no longer works after installing a scoped plugin")
This PR changes approach - such plugins are now considered to have the id `@akdor1154/some-plugin`, in agreement with how npm treats such packages. This allows almost all special cases for scoped packages to be removed (yay). The key difference in behaviour as a result of this, though, is that while plugins are still installed in `plugins_dir/[plugin_id]`, `[plugin_id]` may no longer be a single directory, leading to plugin directories that look like
```
plugins
|- @akdor1154
| |- some-plugin
|- cordova-some-other-plugin
```
Most of the logic changes in this PR are based around making this change work.
It's largely done how I want it, but I guess maintainers probably have strong opinions over whether this is the right way to go or not. Because of this I've left some commits in marked as TEMP that are for my own workflow. Please keep in mind I'll remove these. The only one you should be mindful of is the monkey patch to `cordova-common`; this would need to be raised in a separate PR to cordova-common if this change was approved in principle.
### Platforms affected
All
### What testing has been done on this change?
New end-to-end integration tests including a proper scoped plugin fixture
Unit tests on scoped plugins
Unit tests for plugman metadata
### Checklist
- [x] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database
- [x] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
- [x] Added automated test coverage as appropriate for this change.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/akdor1154/cordova-lib simple-scopes
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-lib/pull/602.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #602
----
commit 9c1c320d72bf2860f95bd9330ffd7718cdabba82
Author: Jarrad Whitaker <jw...@officeworks.com.au>
Date: 2017-10-20T06:55:44Z
change scope behaviour to consider scopes to be part of the package name
commit 67dbb75fa96de8486418dd0a0bfbaa22300690d5
Author: Jarrad Whitaker <jw...@officeworks.com.au>
Date: 2017-10-20T07:07:56Z
remove extraneous done callback from scope plugin tests
commit c6f681cc4df5fd4a5bd246df00a627532111144c
Author: Jarrad Whitaker <jw...@officeworks.com.au>
Date: 2017-10-20T08:40:08Z
add scoped plugin testcase
commit c2346e062518c639381ac167ba60a169500501eb
Author: Jarrad Whitaker <jw...@officeworks.com.au>
Date: 2017-10-20T08:41:46Z
allow scoped plugins to exist in a dir structure reflecting there name, c.f. npm
commit 14c3f74a0d9033dfb250bc7b698e18eb226bf544
Author: Jarrad Whitaker <jw...@officeworks.com.au>
Date: 2017-10-20T08:42:07Z
add an integration test to check scoped plugin add+remove
commit 0e383f9b419c5aabbbb22bbacb456c0ff38649e1
Author: Jarrad Whitaker <jw...@officeworks.com.au>
Date: 2017-10-20T08:43:14Z
TEMP lint with typescript
commit 7018c13a635b46364b34cf4378c78c60a1012f8b
Author: Jarrad Whitaker <jw...@officeworks.com.au>
Date: 2017-10-20T08:42:27Z
TEMP override plugin discovery in cordova-common
commit 7aad7297f7306b380761e01017c067b31beca4a7
Author: Jarrad Whitaker <jw...@officeworks.com.au>
Date: 2017-10-23T06:23:14Z
fix get_fetch_metadata to not guess plugins_dir anymore
commit 17a8cce029ede8060383f041bbc9735f841556bf
Author: Jarrad Whitaker <jw...@officeworks.com.au>
Date: 2017-10-23T06:53:59Z
remove top_plugins override
commit c0b1f5caae2d75b1bec7052598bf0f5f03285827
Author: Jarrad Whitaker <jw...@officeworks.com.au>
Date: 2017-10-23T23:38:16Z
unit test scopes
commit 419485af86670606313abe097d7bbe220d80b756
Author: Jarrad Whitaker <jw...@officeworks.com.au>
Date: 2017-10-24T04:36:53Z
add metadata unit tests
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request #602: CB-12774 : Don't munge scoped plugin IDs anym...
Posted by audreyso <gi...@git.apache.org>.
Github user audreyso commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/602#discussion_r147012615
--- Diff: src/cordova/util.js ---
@@ -255,11 +255,20 @@ function findPlugins (pluginDir) {
var plugins = [];
if (fs.existsSync(pluginDir)) {
- plugins = fs.readdirSync(pluginDir).filter(function (fileName) {
- var pluginPath = path.join(pluginDir, fileName);
- var isPlugin = isDirectory(pluginPath) || isSymbolicLink(pluginPath);
- return fileName !== '.svn' && fileName !== 'CVS' && isPlugin;
- });
+ plugins = fs.readdirSync(pluginDir)
+ .reduce(function (plugins, pluginOrScope) {
+ if (pluginOrScope[0] === '@') {
+ plugins.push(...fs.readdirSync(path.join(pluginDir, pluginOrScope)).map(s => path.join(pluginOrScope, s)));
--- End diff --
Hi! This line will need to be changed as it will not work with Node 4, which is still currently supported. http://node.green/#ES2015-syntax-spread-------operator-with-generic-iterables--in-calls
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request #602: CB-12774 : Don't munge scoped plugin IDs anym...
Posted by audreyso <gi...@git.apache.org>.
Github user audreyso commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/602#discussion_r147014589
--- Diff: src/cordova/util.js ---
@@ -255,11 +255,20 @@ function findPlugins (pluginDir) {
var plugins = [];
if (fs.existsSync(pluginDir)) {
- plugins = fs.readdirSync(pluginDir).filter(function (fileName) {
- var pluginPath = path.join(pluginDir, fileName);
- var isPlugin = isDirectory(pluginPath) || isSymbolicLink(pluginPath);
- return fileName !== '.svn' && fileName !== 'CVS' && isPlugin;
- });
+ plugins = fs.readdirSync(pluginDir)
+ .reduce(function (plugins, pluginOrScope) {
+ if (pluginOrScope[0] === '@') {
+ plugins.push(...fs.readdirSync(path.join(pluginDir, pluginOrScope)).map(s => path.join(pluginOrScope, s)));
--- End diff --
Thanks for your question! We are sticking to the current style for now. We need to have a discussion on our mailing list about starting to use es2015 features/ which ones to use and get some consensus.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request #602: CB-12774 : Don't munge scoped plugin IDs anym...
Posted by akdor1154 <gi...@git.apache.org>.
Github user akdor1154 commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/602#discussion_r147012928
--- Diff: src/cordova/util.js ---
@@ -255,11 +255,20 @@ function findPlugins (pluginDir) {
var plugins = [];
if (fs.existsSync(pluginDir)) {
- plugins = fs.readdirSync(pluginDir).filter(function (fileName) {
- var pluginPath = path.join(pluginDir, fileName);
- var isPlugin = isDirectory(pluginPath) || isSymbolicLink(pluginPath);
- return fileName !== '.svn' && fileName !== 'CVS' && isPlugin;
- });
+ plugins = fs.readdirSync(pluginDir)
+ .reduce(function (plugins, pluginOrScope) {
+ if (pluginOrScope[0] === '@') {
+ plugins.push(...fs.readdirSync(path.join(pluginDir, pluginOrScope)).map(s => path.join(pluginOrScope, s)));
--- End diff --
yup just started looking at the failed test :) While you're here and kind of related, I noticed most of the Cordova codebase avoids `() => arrow functions`. Is this just historical? I have used full `function () { }` when I've paid attention just to match code style, but is this necessary?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org