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

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

Closed
wants to merge 14 commits into from

Conversation

josieusa
Copy link
Collaborator

@josieusa josieusa commented Sep 9, 2016

Messed up this PR, replaced with PR #11
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 # 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.

bajtos and others added 9 commits August 2, 2016 14:41
As soon as CLS module is loaded, the instrumentation/patching of
async-listener is fired. This may cause stack overflows due to promise
instrumentation.

By loading CLS module lazily (only when used for real), we avoid
this kind of problems in applications that are not using current-context
at all.
Rename "per-request-context" middleware to shorter "per-request".

Introduce "LoopBackContext.perRequest" as a shortcut for way too long
  "loopback-context/server/middlewar/per-request"
Recommend against using this module.
 * First release!
@slnode
Copy link

slnode commented Sep 9, 2016

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

@slnode
Copy link

slnode commented Sep 9, 2016

Can one of the admins verify this patch?

@josieusa
Copy link
Collaborator Author

josieusa commented Sep 9, 2016

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 [email protected]

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.

@josieusa
Copy link
Collaborator Author

josieusa commented Sep 9, 2016

I messed with rebase while trying to sync the fork with the upstream repository. I'm going to replace this with another PR with the same changes by me.

@josieusa josieusa changed the title [WIP] Completely and permanently replace continuation-local-storage with cls-hooked [DELETE THIS][WIP] Completely and permanently replace continuation-local-storage with cls-hooked Sep 9, 2016
@josieusa josieusa changed the title [DELETE THIS][WIP] Completely and permanently replace continuation-local-storage with cls-hooked [CLOSE THIS][WIP] Completely and permanently replace continuation-local-storage with cls-hooked Sep 9, 2016
@josieusa josieusa closed this Sep 9, 2016
@josieusa josieusa deleted the feature/cls-hooked-2 branch September 9, 2016 13:24
@slnode
Copy link

slnode commented Sep 9, 2016

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

@slnode
Copy link

slnode commented Sep 9, 2016

Can one of the admins verify this patch?

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

Successfully merging this pull request may close these issues.

5 participants