Hotfix/scoped package support #3453

daankets · 2018-02-07T07:36:01Z

Please always submit pull requests on the development branch.

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	sometimes - issue on Node 6 (tests fail every now and then)
Fixed tickets	#3451
License	MIT
Doc PR	https://github.com/pm2-hive/pm2-hive.github.io/pulls

This fix ensures that you can install scoped modules correctly. It's mainly a fix for the canonicalisation of the module name, when it has a form of @scope/name. Before, the routine that separated the module name separated at the last slash. Secondly, the module name was incorrectly split (assuming the @ was the start of a tag).

Note: Never mind the 2nd commit. It was reverted during the merge. I originally tested locally with an increased hotfix version.

…otfix/scoped-package-support # Conflicts: # package.json

CLAassistant · 2018-02-07T07:36:09Z

All committers have signed the CLA.

wallet77 · 2018-02-07T17:05:22Z

Hi @daankets,

I'm waiting for your feedback about my comments, I think tests will passed after some changes.

Thank you for your contribution !

daankets · 2018-02-08T08:07:13Z

@wallet77 Where can I find these comments?? I didn't get any notification nor can I find anything in this thread...

wallet77 · 2018-02-07T14:27:07Z

test/interface/utility.mocha.js

@@ -18,7 +18,7 @@ describe('Utility', function() {
      assert(Utility.getCanonicModuleName('ma-zal/pm2-slack') === 'pm2-slack');
      assert(Utility.getCanonicModuleName('ma-zal/pm2-slack#own-branch') === 'pm2-slack');
      assert(Utility.getCanonicModuleName('pm2-slack') === 'pm2-slack');
-      assert(Utility.getCanonicModuleName('@org/pm2-slack') === 'pm2-slack');
+      assert(Utility.getCanonicModuleName('@org/pm2-slack') === '@org/pm2-slack');


Could you just add a test with multiple @.
Like @org/[email protected]
Just to ensure we test all cases.

wallet77 · 2018-02-07T17:03:34Z

lib/Utility.js

-    if(canonic_module_name.indexOf('@') !== -1) {
-      canonic_module_name = canonic_module_name.split('@')[0];
+    //pm2 install @somescope/[email protected]
+    if(canonic_module_name.lastIndexOf('@') !== 0) {


I think we need to have > 0 instead of !==0.
Otherwise if you have a simple module name (without @, / etc) condition will be evaluated as true because lastIndexOf('@') will return -1.
This is why tests are failing.

wallet77 · 2018-02-08T09:24:59Z

Sorry my mistake, review wasn't submitted ;)

- Additional test case for @scope/module@tag - Fixed a bug for checking the position of the last @ in the module name, that could trigger on -1.

daankets · 2018-02-08T10:25:12Z

@wallet77 I've updated the pull after your remarks (agree with both, but I did test the case with @scope/module@additionalTag though ;-))

wallet77 · 2018-02-08T10:56:02Z

lib/Utility.js

@@ -223,12 +223,14 @@ var Utility = module.exports = {

    //pm2 install username/module
    else if(canonic_module_name.indexOf('/') !== -1) {
-      canonic_module_name = canonic_module_name.split('/')[1];
+      if (!canonic_module_name.startsWith("@")){


Tests on 0.12 failed cause startsWidth is not supported :(
I think we should write test just like this :
if (canonic_module_name[0] !== "@"){

daankets · 2018-02-08T10:59:00Z

Yep, I got that. Didn’t immediately look at the Node versions. Will fix this.

…

On 8 Feb 2018, at 11:56, Vincent Vallet ***@***.***> wrote: @wallet77 commented on this pull request. In lib/Utility.js <#3453 (comment)>: > @@ -223,12 +223,14 @@ var Utility = module.exports = { //pm2 install username/module else if(canonic_module_name.indexOf('/') !== -1) { - canonic_module_name = canonic_module_name.split('/')[1]; + if (!canonic_module_name.startsWith("@")){ Tests on 0.12 failed cause startsWidth is not supported :( I think we should write test just like this : if (canonic_module_name[0] !== "@"){ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3453 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACLKMQq-rFd6jZfw8CW8zYkQS_GxX5Xdks5tStLagaJpZM4R8Q1D>.

-- *Disclaimer* This message is *confidential* and only intended for the originally intended recipient(s). It may also be privileged or otherwise protected by work product immunity or other legal rules. *If you think you have received this message by mistake*, please let us know by e-mail reply and delete it from your system; you must not copy this message or disclose its contents to anyone other than the intended recipients.

daankets · 2018-02-08T12:11:10Z

@wallet77 The build now fails on Node 6 only, but I don't know the code base enough to understand why (without a hint). It's in the aggregator mocha tests. They rune fine on other node versions though.

daankets · 2018-02-08T12:13:43Z

@wallet77 Just noticed they don't ALWAYS fail. If I rerun the tests on node 6, they succeed more than they fail.

daankets · 2018-02-08T12:26:29Z

This test failed on the CI with node 6. However, it runs fine here (90% of the time)

/usr/local/opt/node@6/bin/node /Users/daankets/Projects/pm2/node_modules/mocha/bin/_mocha --ui bdd --reporter "/Users/daankets/Library/Application Support/JetBrains/Toolbox/apps/WebStorm/ch-0/173.4548.30/WebStorm.app/Contents/plugins/NodeJS/js/mocha-intellij/lib/mochaIntellijReporter.js" /Users/daankets/Projects/pm2/test/interface/aggregator.mocha.js --grep "Transactions Aggregator "
2018-02-08T12:24:39.205Z pm2:aggregator spans is null
2018-02-08T12:24:39.333Z pm2:aggregator Packet malformated null
2018-02-08T12:24:39.455Z pm2:aggregator Reseting agg for app:appname meta:{}
2018-02-08T12:24:39.467Z pm2:aggregator Reseting agg for app:appname meta:{"name":"appname"}
2018-02-08T12:24:39.467Z pm2:aggregator Reseting initialization timeout app:appname
2018-02-08T12:24:39.469Z pm2:aggregator Reseting agg for app:APP2 meta:{"name":"APP2","server":"test","rev":"xxx"}

Process finished with exit code 0

wallet77 · 2018-02-08T13:06:01Z

@daankets if they passed one time then it's ok.
Indeed sometimes travis failed but it's not related to your PR.
I will merge it when all tests are green :)

daankets · 2018-02-08T13:37:00Z

@wallet77 Thanks. Any idea when the next release is planned? I can't wait to start using this feature ;-)

wallet77 · 2018-02-08T13:41:56Z

No precise date for now but during February I think.
@Unitech when do we expect a release ?

…e-support Hotfix/scoped package support

daankets added 3 commits February 7, 2018 09:13

Support for scoped package names

a3e093b

Hotfix version increased

5968661

Merge branch 'development' of ssh://github.com/deltasource/pm2 into h…

94ea9d9

…otfix/scoped-package-support # Conflicts: # package.json

wallet77 reviewed Feb 8, 2018

View reviewed changes

Added one test case, fixed a small bug

1c58bf4

- Additional test case for @scope/module@tag - Fixed a bug for checking the position of the last @ in the module name, that could trigger on -1.

wallet77 reviewed Feb 8, 2018

View reviewed changes

Fix for node 0.12 test (no String.startsWith())

ace8d0c

wallet77 merged commit 974f9bf into Unitech:development Feb 8, 2018

daankets deleted the hotfix/scoped-package-support branch February 8, 2018 13:28

inerc pushed a commit to inerc/pm2 that referenced this pull request Feb 11, 2020

Merge pull request Unitech#3453 from deltasource/hotfix/scoped-packag…

1cb685d

…e-support Hotfix/scoped package support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hotfix/scoped package support #3453

Hotfix/scoped package support #3453

daankets commented Feb 7, 2018 •

edited

Loading

CLAassistant commented Feb 7, 2018 •

edited

Loading

wallet77 commented Feb 7, 2018

daankets commented Feb 8, 2018

wallet77 Feb 7, 2018

wallet77 Feb 7, 2018

wallet77 commented Feb 8, 2018

daankets commented Feb 8, 2018

wallet77 Feb 8, 2018

daankets commented Feb 8, 2018 via email

daankets commented Feb 8, 2018

daankets commented Feb 8, 2018

daankets commented Feb 8, 2018

wallet77 commented Feb 8, 2018

daankets commented Feb 8, 2018

wallet77 commented Feb 8, 2018

Hotfix/scoped package support #3453

Hotfix/scoped package support #3453

Conversation

daankets commented Feb 7, 2018 • edited Loading

CLAassistant commented Feb 7, 2018 • edited Loading

wallet77 commented Feb 7, 2018

daankets commented Feb 8, 2018

wallet77 Feb 7, 2018

Choose a reason for hiding this comment

wallet77 Feb 7, 2018

Choose a reason for hiding this comment

wallet77 commented Feb 8, 2018

daankets commented Feb 8, 2018

wallet77 Feb 8, 2018

Choose a reason for hiding this comment

daankets commented Feb 8, 2018 via email

daankets commented Feb 8, 2018

daankets commented Feb 8, 2018

daankets commented Feb 8, 2018

wallet77 commented Feb 8, 2018

daankets commented Feb 8, 2018

wallet77 commented Feb 8, 2018

daankets commented Feb 7, 2018 •

edited

Loading

CLAassistant commented Feb 7, 2018 •

edited

Loading