[SEMVER-MAJOR] Replace continuation-local-storage with cls-hooked #11

josieusa · 2016-09-09T13:21:15Z

Hello, this is a quick and (not so) dirty alternative to PR #2.

Rationale:

Personally, I still like more the approach I used in PR [WIP] Use cls-hooked instead of continuation-local-storage #2, but I just realized it needs further analysis, because I initially forgot to take into account issue async-listener#57.
The quickest and most obvious way to address this is, of course, to get rid of async-listener, which already was the long-term goal of my previous PR, anyway. Here it just turned into an immediate goal, that's all.
One trivial way to reach this goal immediately is to completely replace continuation-local-storage with cls-hooked quitting any attempt to keep backward compatibility with the former, which is exactly what I did in this PR and is the difference with my previous PR. This should make the commit df60f13no longer necessary, so I reverted it.
cls-hooked, however (maybe continuation-local-storage too, I don't care), needs to be required before everything else, in order to not fail silently and catastrophically. So, I also gave proper and detailed instructions about how to do this in the README.md

Anyway, given the very short time I dedicated to this PR, I would consider this a work-in-progress for now.

TODO:

Bump version to 2.0.0-alpha.1 when it's more ready for alpha release.

PS
For now, in order to make tests work, after deleting node_modules folder and running npm i, I had to run (NPM v3):

npm r strong-remoting
npm i strong-remoting@3.0.0-alpha.5

I suppose this is because loopback PR #2696 maybe hasn't landed in loopback-context yet.

Also, for the same reason, if you test this branch from inside a Loopback app, make sure that loopback-context uses the same version of strong-remoting as the app.

slnode · 2016-09-09T13:21:16Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

slnode · 2016-09-09T13:21:16Z

Can one of the admins verify this patch?

bajtos · 2016-09-13T10:46:33Z

CHANGES.md

@@ -2,3 +2,8 @@
 =========================

 * First release!
+
+2016-08-10, Version 2.0.0-alpha.1


The contents in CHANGES.md is generated by our release tooling, please revert this change.

josieusa · 2016-09-13T15:49:27Z

Ok, I addressed all the line notes from the mantainer; but before removing the [WIP] in the title I'd like to try this in production, despite having some successful tests already in place. I'll let you know soon.

bajtos · 2016-09-14T14:32:53Z

Ok, I addressed all the line notes from the mantainer; but before removing the [WIP] in the title I'd like to try this in production, despite having some successful tests already in place. I'll let you know soon.

Sounds good, thank you!

evseevnn-zz · 2016-10-07T13:53:56Z

How soon this come?
I tested code from josieusa:feature/cls-hooked-3
This not work!
I did it like writen in readme.md

Maybe you can prepare worked example or change readme.md?

UPD. Sorry guys! This work, but problem different, just i use loopback-connector-mongodb@1.15.2 module. Explaination here

bajtos · 2016-11-22T08:03:01Z

FWIW, #14 has dropped support for Node v0.10 and Node v0.12. There are no blockers now that would prevent us from landing an implementation based on cls-hooked.

slnode · 2016-11-22T08:03:03Z

Can one of the admins verify this patch?

josieusa · 2016-12-09T14:14:43Z

@bajtos Sorry for the late response! Luckily, I'm testing this in production right now, and I'll tell you if it works for me ASAP.
I'd only mention that in production I'm using Node 6 and loopback 2, but if it works on 2, I don't see any reason why it shouldn't work on LB 3, since the legacy context feature is deprecated on both versions (anyway, Mocha tests are run on LB 3 since it's a dev dependency).
Just one caveat: if the app uses Node 4, other stuff wouldn't break because of this fix, but if the app uses the current context API, the legacy context feature would be used as a fallback, which is not (and likely will never be) fixed by this PR.
TL;DR whoever wishes to use the working current context feature in this PR, should update to Node 6 before.
Thank you.

bajtos · 2016-12-09T14:45:11Z

TL;DR whoever wishes to use the working current context feature in this PR, should update to Node 6 before.

In that case, let's bump up engines.node field in package.json to >=6 (or whatever minimal Node.js version is required for your new code to work correctly). If the fallback mechanism is never used on Node v6 (and newer), then I am fine with removing this fallback mechanism completely and perhaps throwing an error if the required Node.js API is not available.

@bajtos

As per suggestion of @bajtos in comment #266030074 in PR strongloop#11

josieusa · 2016-12-16T09:48:42Z

Hello,
I updated the engines.node field to ^4.5 || ^5.10 || ^6.0 instead of just 6.0 because this is the exact version range required by async-hook dependency of this fork (from its package.json).
My tests in production with Node 6 and Loopback 2 are going fine, and they will end around Tuesday.
After the end of the tests, I would like to use this fork permanently in production.
So, since there are no more line notes left to address, what do you think about a merge when my tests end successfully?
I don't need to add more commits, maybe just some edits to README before Tuesday.
Thank you.

bajtos

PTAL 👇

bajtos · 2016-12-16T15:39:55Z

README.md

+npm test
+```
+
+This adds the `-r` option to `mocha` command, needed in order to pass tests.


Please move the -r option to test/mocha.opts file, so that people can run tests using mocha directly. I personally use this frequently, e.g. to run only a single test via mocha -g "test name".

--require cls-hooked

(BTW you pull request does not change package.json despite what README says.)

Ah, I see that you already have test/mocha.opts. I think we should simply remove this section from README then.

bajtos · 2016-12-16T15:40:32Z

package.json

@@ -1,9 +1,9 @@
 {
  "name": "loopback-context",
-  "version": "3.0.0-alpha.1",


Please don't change the version number, we will bump it when releasing a new version.

bajtos · 2016-12-16T15:50:04Z

test/main.test.js

+      var asyncFn = function() {
+        async.waterfall([
+          fn1,
+          fn2,


FWIW, we usually structure our code differently:

async.waterfall([ function pushToContext(next) { // code from fn1 }, function pullFromContext(next) { // code from fn2 }, function verify(next) { expect(testValue).to.equal('test-value'); next(); }, ], done);

@bajtos

See @bajtos comment in PR strongloop#11 strongloop#11 (comment)

josieusa · 2016-12-20T13:49:59Z

Hello,
I confirm it works for me using both usage modes, that is, both node -r cls-hooked . and require('cls-hooked'). Issues like strongloop/loopback#2397, strongloop/loopback#2519, strongloop/loopback#2485 didn't occur to me yet since using this fork (UPDATE: except if using when package, see this issue). ~~Though the LB issue 2485 should be load-tested.~~ Someone did a load test here, resulting in improvements but no solution to the when issue, though he hasn't shared test code yet. Because of this, I'll keep using the fork in production with shrinkwrap, in an average-size app with hundreds of users, regardless of whether it gets merged or not.
Thank you

bajtos · 2016-12-21T09:38:39Z

@josieusa awesome, thank you for the update! I am not sure if I'll have time to review and land this before I leave for vacation.

dehypnosis · 2016-12-27T19:13:15Z

lose a lot of time again. sad.
i have no idea that how many people using loopback deals with current context problems..

josieusa · 2016-12-28T07:46:05Z

@dehypnosis If you want to use this in your app before it gets merged, you can do like this in package.json if your NPM version is recent enough:

...
"dependencies": {
...
    "loopback-context": "github:josieusa/loopback-context#feature/cls-hooked-3",
...
},
...

If you do this, I strongly recommend to edit package.json again afterwards, as soon as this PR gets merged.
Please remember that this PR is not considered to be the official solution (though it works fine for me).

Rework the module to use the new cls-hooked module (which uses AsyncWrap available since Node v4.5) instead of old continuation-local-storage (based on very unreliable async-listener).

bajtos

The patch looks good to me now. In preparation for landing it, I have cleaned up the commit history and git push -f to your feature branch.

bajtos · 2017-01-03T12:50:22Z

Landed, thank you for your contribution 🎉 and for keeping the ball rolling despite the long time it took to get this work done 🙇

michaelfreund · 2017-01-25T11:56:20Z

@bajtos i think the version number is confusing since 3.0.0 seems to be related to LB3. I guess this is wrong. After some tests it seems that LB2.x projects should also include loopback-context@3.0.0 to get this changes.

bajtos · 2017-01-25T13:33:39Z

After some tests it seems that LB2.x projects should also include loopback-context@3.0.0 to get this changes.

Yes, you should be able to use loopback-context@3.0.0 in LB2.x projects too.

i think the version number is confusing since 3.0.0 seems to be related to LB3. I guess this is wrong.

I am afraid it would be similarly confusing if we used 2.0.0, because it would hint relation to LB2.

We can improve the documentation though to make this more clear. Can you contribute this change yourself please? @michaelfreund

michaelfreund · 2017-01-27T05:06:10Z

@bajtos I totally agree with the version number. Of course i can contribute. What do you think of including related component information in the docs? Like for the context stuff

Loopback 3.x
Latest Component Version: 3.0.0
Loopback 2.x
Latest Component Version: 3.0.0
Source
https://github.com/strongloop/loopback-context

BTW http://loopback.io/doc/en/lb3/Using-current-context.html#use-a-custom-strong-remoting-phase seems also working in LB2.x but is missing in the docs. Can you confirm? So i can update that too.

bajtos · 2017-02-17T14:55:20Z

@michaelfreund sorry for responding late.

What do you think of including related component information in the docs? Like for the context stuff

I am proposing to add a new section to loopback-context's README with a table similar to
https://github.com/strongloop/loopback-component-explorer#supported-versions

BTW http://loopback.io/doc/en/lb3/Using-current-context.html#use-a-custom-strong-remoting-phase seems also working in LB2.x but is missing in the docs. Can you confirm? So i can update that too.

Yes, custom remoting phases are available in LB 2.x too.

crandmck added the #community contribution label Sep 9, 2016

josieusa mentioned this pull request Sep 9, 2016

[CLOSE THIS][WIP] Completely and permanently replace continuation-local-storage with cls-hooked #10

Closed

josieusa force-pushed the feature/cls-hooked-3 branch 4 times, most recently from f8b0d71 to b91ed98 Compare September 9, 2016 15:18

josieusa mentioned this pull request Sep 9, 2016

Community contributions to keep mantaining deprecated current context feature strongloop/loopback#2728

Closed

josieusa force-pushed the feature/cls-hooked-3 branch 3 times, most recently from 6371337 to 8feccc3 Compare September 12, 2016 12:39

bajtos reviewed Sep 13, 2016
View reviewed changes

josieusa force-pushed the feature/cls-hooked-3 branch from 9738804 to e597ef9 Compare December 9, 2016 14:39

josieusa force-pushed the feature/cls-hooked-3 branch 2 times, most recently from 6c8a470 to f0bb654 Compare December 9, 2016 15:53

josieusa pushed a commit to josieusa/loopback-context that referenced this pull request Dec 9, 2016

Bump engines.node in package.json to >=4.5

defba32

As per suggestion of @bajtos in comment #266030074 in PR strongloop#11

josieusa force-pushed the feature/cls-hooked-3 branch from defba32 to f0bb654 Compare December 9, 2016 16:30

bajtos mentioned this pull request Dec 16, 2016

Inject remoting context to options arg strongloop/loopback#3023

Merged

3 tasks

bajtos requested changes Dec 16, 2016

View reviewed changes

josieusa added a commit to josieusa/loopback-context that referenced this pull request Dec 19, 2016

Follow strongloop style in test

538f577

See @bajtos comment in PR strongloop#11 strongloop#11 (comment)

bajtos changed the title ~~[WIP] Completely and permanently replace continuation-local-storage with cls-hooked~~ [SEMVER-MAJOR] Replace continuation-local-storage with cls-hooked Dec 19, 2016

josieusa mentioned this pull request Dec 21, 2016

[WIP] Use cls-hooked instead of continuation-local-storage #2

Closed

josieusa mentioned this pull request Dec 22, 2016

Injecting Remote Context via Options strongloop/loopback#1495

Closed

4 tasks

Drop continuation-local-storage, use cls-hooked

996a49f

Rework the module to use the new cls-hooked module (which uses AsyncWrap available since Node v4.5) instead of old continuation-local-storage (based on very unreliable async-listener).

bajtos force-pushed the feature/cls-hooked-3 branch from 779a5ba to 996a49f Compare January 3, 2017 12:25

bajtos approved these changes Jan 3, 2017

View reviewed changes

bajtos merged commit db9387e into strongloop:master Jan 3, 2017

bajtos removed the #community contribution label Jan 3, 2017

josieusa mentioned this pull request Jan 4, 2017

MongoDB - loopback.getCurrentContext() return null strongloop/loopback#2519

Closed

bajtos mentioned this pull request Jan 5, 2017

MongoDB - loopback.getCurrentContext() return null #21

Closed

josieusa mentioned this pull request Jan 5, 2017

Release 3.0.0 #20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SEMVER-MAJOR] Replace continuation-local-storage with cls-hooked #11

[SEMVER-MAJOR] Replace continuation-local-storage with cls-hooked #11

josieusa commented Sep 9, 2016

slnode commented Sep 9, 2016

slnode commented Sep 9, 2016

bajtos Sep 13, 2016

josieusa commented Sep 13, 2016

bajtos commented Sep 14, 2016

evseevnn-zz commented Oct 7, 2016 •

edited

Loading

bajtos commented Nov 22, 2016

slnode commented Nov 22, 2016

josieusa commented Dec 9, 2016 •

edited

Loading

bajtos commented Dec 9, 2016

josieusa commented Dec 16, 2016

bajtos left a comment

bajtos Dec 16, 2016

bajtos Dec 16, 2016

bajtos Dec 16, 2016

bajtos Dec 16, 2016

josieusa commented Dec 20, 2016 •

edited

Loading

bajtos commented Dec 21, 2016

dehypnosis commented Dec 27, 2016 •

edited

Loading

josieusa commented Dec 28, 2016 •

edited

Loading

bajtos left a comment

bajtos commented Jan 3, 2017

michaelfreund commented Jan 25, 2017

bajtos commented Jan 25, 2017 •

edited

Loading

michaelfreund commented Jan 27, 2017 •

edited

Loading

bajtos commented Feb 17, 2017

[SEMVER-MAJOR] Replace continuation-local-storage with cls-hooked #11

[SEMVER-MAJOR] Replace continuation-local-storage with cls-hooked #11

Conversation

josieusa commented Sep 9, 2016

slnode commented Sep 9, 2016

slnode commented Sep 9, 2016

bajtos Sep 13, 2016

Choose a reason for hiding this comment

josieusa commented Sep 13, 2016

bajtos commented Sep 14, 2016

evseevnn-zz commented Oct 7, 2016 • edited Loading

bajtos commented Nov 22, 2016

slnode commented Nov 22, 2016

josieusa commented Dec 9, 2016 • edited Loading

bajtos commented Dec 9, 2016

josieusa commented Dec 16, 2016

bajtos left a comment

Choose a reason for hiding this comment

bajtos Dec 16, 2016

Choose a reason for hiding this comment

bajtos Dec 16, 2016

Choose a reason for hiding this comment

bajtos Dec 16, 2016

Choose a reason for hiding this comment

bajtos Dec 16, 2016

Choose a reason for hiding this comment

josieusa commented Dec 20, 2016 • edited Loading

bajtos commented Dec 21, 2016

dehypnosis commented Dec 27, 2016 • edited Loading

josieusa commented Dec 28, 2016 • edited Loading

bajtos left a comment

Choose a reason for hiding this comment

bajtos commented Jan 3, 2017

michaelfreund commented Jan 25, 2017

bajtos commented Jan 25, 2017 • edited Loading

michaelfreund commented Jan 27, 2017 • edited Loading

bajtos commented Feb 17, 2017

evseevnn-zz commented Oct 7, 2016 •

edited

Loading

josieusa commented Dec 9, 2016 •

edited

Loading

josieusa commented Dec 20, 2016 •

edited

Loading

dehypnosis commented Dec 27, 2016 •

edited

Loading

josieusa commented Dec 28, 2016 •

edited

Loading

bajtos commented Jan 25, 2017 •

edited

Loading

michaelfreund commented Jan 27, 2017 •

edited

Loading