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

Upgrade dependencies to remove npm install warnings #1228

Merged
merged 1 commit into from
Jan 19, 2017
Merged

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Jan 17, 2017

Description

Upgrade dependencies to remove warnings from npm install.

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

@raymondfeng the changes themselves looks good, however I am concerned about possible breaking changes caused by upgrading async and qs 👇

},
"dependencies": {
"async": "~1.0.0",
"async": "~2.1.4",
Copy link
Member

Choose a reason for hiding this comment

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

Do we know what were the breaking changes between async 1.x and 2.x? Can they cause a breaking change in juggler?

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember 1 breaking change was related to loopback current context.

Copy link
Member

Choose a reason for hiding this comment

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

The list of breaking changes can be found here: https://github.com/caolan/async/blob/d3f583a1d0e6d248b3dc5adbe20ac1c4ccdb92c5/CHANGELOG.md#breaking-changes

TL;DR: I think we are good to upgrade here.

Here are the selected items that I think may affect us:

  1. Calling a callback more than once is considered an error, and an error will be thrown. This had an explicit breaking change in waterfall. If you were relying on this behavior, you should more accurately represent your control flow as an event emitter or stream.

  2. Internal setImmediate calls have been refactored away. This may make existing flows vulnerable to stack overflows if you use many synchronous functions in series.

  3. filter, reject, some, every, and related functions now expect an error as the first callback argument, rather than just a simple boolean. Pass null as the first argument

(1) and (3) can cause bugs where juggler is relying on the old async@1 behaviour. Hopefully, such problems would be caught by our unit tests, so we should be good here.

(2) is more subtle. juggler was originally written with async@0.x, which was not calling setImmediate. IIRC, async@1 added setImmediate calls, and this broke CLS/getCurrentContext. Now that async@2 is removing (some of) setImmediate calls, I think our code should not be affected, because it was written with the expectation that setImmediate is not called at all.

"minimatch": "^3.0.3",
"node-uuid": "^1.4.2",
"qs": "^3.1.0",
"qs": "^6.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

Ditto, did you assess the impact of this change?

Copy link
Member

Choose a reason for hiding this comment

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

AFAICT, qs is used only to parse dataSource/connector settings when a single URL string is provided.

qs's changelog can be found here: https://github.com/ljharb/qs/blob/8bd4c6cf12898f469838980317fec92007e5112a/CHANGELOG.md

Here is an overview of breaking changes:

As I see it, the only thing that may break after this update is connection strings using key.subkey=value notation instead of key[subkey]=value. Considering that the dot notation is not documented in LoopBack docs, I think upgrade to qs@4+ shouldn't cause any significant breakage of existing apps.

@bajtos
Copy link
Member

bajtos commented Jan 19, 2017

@raymondfeng I have looked up the release notes for you, see my comments above. I think this patch is safe to be landed to master (3.x). I think we shouldn't be upgrading 2.x LTS though.

@raymondfeng raymondfeng merged commit 6d7ae88 into master Jan 19, 2017
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.

None yet

3 participants