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

Deprecate current-context API [2.x] #2559

Merged
merged 2 commits into from
Aug 10, 2016
Merged

Deprecate current-context API [2.x] #2559

merged 2 commits into from
Aug 10, 2016

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Jul 28, 2016

Deprecate all current-context APIs in favour of loopback-context-cls.

This is a follow-up for strongloop/loopback-context#1

Based on our discussions, I think we should remove getCurrentContext from LoopBack 3.0. In order to simplify upgrade path and allow community to take over maintenance of CLS-based current context, I want to follow the same process we did with loopback-boot and that is to extract the functionality into a standalone module that can be versioned independently.

@raymondfeng @ritch please review
cc @strongloop/loopback-dev FYI

@richardpringle
Copy link
Contributor

Would this somehow make it any easier to solve the following issue without a hack?

#1495

@bajtos
Copy link
Member Author

bajtos commented Jul 28, 2016

Probably not. My intention here is to allow us to back out of getCurrentContext and stop trying to fix it, while not breaking all apps that may be depending on this feature. I think getCurrentContext may work in certain cases, so why to break them if there is a reasonably easy workaround (proposed here).

// ClsContext.getCurrentContext() to loopback.getCurrentContext()
deprecated('loopback.getCurrentContext() is deprecated. ' +
'Consider using different means of passing the context around. ' +
'As a short-term fix, use loopback-context-cls module.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message is a bit confusing. Maybe we should say something like:

The current implementation is based on loopback-context-cls and it has some limitations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine to improve the message, as long as it remains actionable. I.e. it should be clear to the user how to get rid of that deprecation warning.

IMO, your proposal @raymondfeng, describes the state but does not provide any clue to the user about how to proceed.

Which part of the message do you find confusing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we link to a github issue or wiki (or other doc) so that we can keep the info up to date?

A deprecation warning will be sufficient to direct the person to a doc with some concrete info. We can also keep this up to date as we look into this further.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The confusing part of As a short-term fix, use loopback-context-cls module. is that it seems to instruct a user to use loopback-context-cls, which is already being used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reworded deprecation warnings to point to our documentation page: https://docs.strongloop.com/display/APIC/Using%20current%20context

@bajtos bajtos force-pushed the deprecate/getCurrentContext branch from 5c89b50 to ba55e26 Compare July 29, 2016 07:52
@bajtos
Copy link
Member Author

bajtos commented Jul 29, 2016

@raymondfeng @ritch PTAL again, LGTY now?

@raymondfeng
Copy link
Member

LGTM

@bajtos
Copy link
Member Author

bajtos commented Aug 9, 2016

I am waiting for https://github.com/strongloop/loopback-context/pull/3/files before landing this PR.

Deprecate all current-context APIs in favour of loopback-context-cls.
@bajtos bajtos force-pushed the deprecate/getCurrentContext branch from ba55e26 to ca28e7f Compare August 10, 2016 08:59
@bajtos bajtos merged commit bb1af5a into 2.x Aug 10, 2016
@bajtos bajtos deleted the deprecate/getCurrentContext branch August 10, 2016 11:29
@bajtos bajtos removed the #review label Aug 10, 2016
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.

4 participants