Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow scoped Cordova packages to be installed #9334

Closed
wants to merge 8 commits into from

Conversation

@hwillson
Copy link
Member

@hwillson hwillson commented Nov 9, 2017

Hi all - this PR is a continuation of the closed PR #7350, that fixes an issue preventing the installation of scoped Cordova packages. For example, meteor add cordova:@somescope/some-cordova-plugin@1.0.0 will now work properly.

I've updated the closed PR to include the requested code review changes (with a few small modifications), and added some tests.

Fixes #7336. Thanks!

jackkav and others added 5 commits Jul 4, 2016
Fixes an issue preventing the installation of scoped Cordova
packages. For example,
`meteor add cordova:@somescope/some-cordova-plugin@1.0.0`
will now work properly.

Fixes #7336.
packageName: parts[3],
versionSeparator: parts[4],
version: parts[5],
};

This comment has been minimized.

@benjamn

benjamn Nov 9, 2017
Member

Maybe skip the details object?

const [
  _matchText,
  scope,
  scopeSeparator,
  packageName,
  versionSeparator,
  version,
] = packageIdAndVersion.match(
  /^(@[^\/]*)?(\/)?([^@]*)(@)?(.*)?/
);

This comment has been minimized.

@hwillson

hwillson Nov 9, 2017
Author Member

@benjamn I actually did exactly that first, but was greeted with the following

TypeError: Cannot read property 'Symbol(Symbol.iterator)' of packageIdAndVersion.match

I decided to switch it around instead of chasing that down further, but I'll investigate. Thanks!

This comment has been minimized.

@hwillson

hwillson Nov 9, 2017
Author Member

And now for the life of me I can't re-create the same error. So, changes coming!

Copy link
Member

@abernix abernix left a comment

Thanks for grabbing this issue!

// some-cordova-plugin@1.0.0
// @somescope/some-cordova-plugin@1.0.0
exports.parse = packageIdAndVersion => {
const package = {};

This comment has been minimized.

@abernix

abernix Nov 9, 2017
Member

Likely best to avoid the use of package as an identifier since it's a "Future Reserved Word" in ECMAScript.

This comment has been minimized.

@hwillson

hwillson Nov 9, 2017
Author Member

Good call - changing - thanks!

const package = {};
if (packageIdAndVersion) {
const parts = packageIdAndVersion.match(
/^(@[^\/]*)?(\/)?([^@]*)(@)?(.*)?/

This comment has been minimized.

@abernix

abernix Nov 9, 2017
Member

/^(@[^\/]*)?(\/)?([^@]*)(@)?(.*)?/
         ^

When the scope is present in this manner, is it required that there be at least a character after the @? If so,+ ("1+ occurrence") would be more appropriate here than * ("0+ occurrences"). Similar question about the other * cases.*

*Winner not guaranteed. ;-)

This comment has been minimized.

@hwillson

hwillson Nov 9, 2017
Author Member

Yes - also a good call! Changes coming.

const parseCordovaIdVersion =
require('../cordova/package-id-version-parser.js').parse;

let package = 'some-cordova-plugin';

This comment has been minimized.

@abernix

abernix Nov 9, 2017
Member

Some package var concern as above.

scope: parts[1],
scopeSeparator: parts[2],
packageName: parts[3],
versionSeparator: parts[4],

This comment has been minimized.

@abernix

abernix Nov 9, 2017
Member

Maybe just not capture versionSeparator?

/^(?:@([^\/]+)\/)?([^\/@]*)@?(.*)?/

This comment has been minimized.

@hwillson

hwillson Nov 9, 2017
Author Member

Sounds good!

const package = {};
if (packageIdAndVersion) {
const parts = packageIdAndVersion.match(
/^(@[^\/]*)?(\/)?([^@]*)(@)?(.*)?/

This comment has been minimized.

@abernix

abernix Nov 9, 2017
Member

/^(@[^\/]*)?(\/)?([^@]*)(@)?(.*)?/
            ^^^^^

Could this be included in the first grouping along with the scope and not explicitly captured? Right now this allows /dd@ and puts / into scopeSeparator with no scope.

Maybe...

/^(@[^\/]*\/?)?([^\/@]*)(@)?(.*)?/

... you'll note I also added / to the negated character class.

This comment has been minimized.

@hwillson

hwillson Nov 9, 2017
Author Member

I split the separator out intentionally; I just liked the idea of having buckets for each individual component, so the scope (when available) was the actual scope name without the trailing /. But, there really isn't any reason to do this, so your point is well taken - changes coming!

packageName: parts[3],
versionSeparator: parts[4],
version: parts[5],
};

This comment has been minimized.

@abernix

abernix Nov 9, 2017
Member

I could probably go further. Is there a way we could avoid the regex and just use a combination of packageIdAndVersion.startsWith('cordova:) and split on packageIdAndVersion.lastIndexOf('@').

This comment has been minimized.

@abernix

abernix Nov 9, 2017
Member

Given the tests, and that you may have already spent some time on this, and that overall this is an improvement, I think it's okay as is too if you just wanted to put your hands up. 🙌

😉

This comment has been minimized.

@hwillson

hwillson Nov 9, 2017
Author Member

I originally avoided the regex and did just that, but the problem is that we need to catch instances where the version number isn't passed in. So the parse function can actually have @somescope/some-cordova-plugin passed in, which means splitting on @ doesn't quite work (since we can have both a valid and invalid package string passed in that has only one @). We can definitely add code to address this, but it's at this point I decided a regex would probably make more sense. Happy to change it if you disagree though!

This comment has been minimized.

@abernix

abernix Nov 9, 2017
Member

Makes sense to me. Thanks for the explanation!

@hwillson
Copy link
Member Author

@hwillson hwillson commented Nov 9, 2017

Review items should now be addressed - thanks guys!

P.S. --> @abernix is a regex master! 🙌 🏆

@benjamn benjamn added this to the Release 1.6.1 milestone Nov 15, 2017
@abernix
Copy link
Member

@abernix abernix commented Nov 17, 2017

Squashed a bit and landed in devel as dd5ea2b and 27ed9bc.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants