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

chore: upgrade/review dependencies and address security alerts #1349

Merged
merged 19 commits into from
Jun 13, 2019

Conversation

ekashida
Copy link
Member

Details

  • Upgrades old dependency versions
  • Removes unused dependencies
  • Addresses remaining moderate and low security alerts (aside from the high alert for cssnano)

Does this PR introduce breaking changes?

  • No, it does not introduce breaking changes.

@ekashida ekashida changed the title Ekashida/dependencies chore: upgrade/review dependencies and address security alerts Jun 10, 2019
@salesforce-best-lwc-internal

This comment has been minimized.

@salesforce-best-lwc-internal

This comment has been minimized.

@salesforce-best-lwc-internal

This comment has been minimized.

@salesforce-best-lwc-internal

This comment has been minimized.

@pmdartus
Copy link
Member

We need to settle, on how we manage the versions of the devDependencies because there is a mix between exact, patch match (~) and minor match (^).

IMO, devDependencies should be versioned using a minor match (^). The yarn.lock ensures the repeatability of the build by freezing all the dependencies (dev or not) to a specific version. Those dependencies are also internal to the project some we can't expect consumers to run into an issue where a package introduces an unintended breaking change without us noticing first. React, Ember and Vue follows the same strategy here. This comes at the exception of:

  • prettier that should be frozen to a specific version, since updating prettier version most of the time requires some code to be reformatted.
  • typescript that should be versioned using patch match (~), because the package removes APIs from the language between minor releases.

I know that @diervo and @caridy will have some strong opinion here =)

@ekashida
Copy link
Member Author

Those are good points. I wasn't sure if we cared about those annotations because the yarn.lock ultimately dictates what gets installed but it would be nice to reflect our expectations in our dependencies. That would, in theory, allow us to simply delete the yarn.lock and do a fresh install to update all our dependencies at once 🤞

@salesforce-best-lwc-internal

This comment has been minimized.

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: c2ca844 | Target commit: e0e5f9e

lwc-engine-benchmark

table-append-1k metric base(c2ca844) target(e0e5f9e) trend
benchmark-table/append/1k duration 148.20 (±4.00 ms) 153.05 (±5.75 ms) +4.9ms (3.3%) 👌
table-clear-1k metric base(c2ca844) target(e0e5f9e) trend
benchmark-table/clear/1k duration 11.10 (±1.05 ms) 12.05 (±1.05 ms) +1.0ms (8.6%) 👎
table-create-10k metric base(c2ca844) target(e0e5f9e) trend
benchmark-table/create/10k duration 906.10 (±6.95 ms) 944.05 (±7.40 ms) +38.0ms (4.2%) 👎
table-create-1k metric base(c2ca844) target(e0e5f9e) trend
benchmark-table/create/1k duration 114.75 (±2.70 ms) 116.55 (±2.95 ms) +1.8ms (1.6%) 👎
table-update-10th-1k metric base(c2ca844) target(e0e5f9e) trend
benchmark-table/update-10th/1k duration 71.00 (±2.00 ms) 82.35 (±2.65 ms) +11.3ms (16.0%) 👎
tablecmp-append-1k metric base(c2ca844) target(e0e5f9e) trend
benchmark-table-component/append/1k duration 217.40 (±16.75 ms) 237.95 (±20.05 ms) +20.5ms (9.5%) 👎
tablecmp-clear-1k metric base(c2ca844) target(e0e5f9e) trend
benchmark-table-component/clear/1k duration 7.30 (±1.20 ms) 8.35 (±1.30 ms) +1.1ms (14.4%) 👌
tablecmp-create-10k metric base(c2ca844) target(e0e5f9e) trend
benchmark-table-component/create/10k duration 1784.20 (±11.35 ms) 1816.05 (±12.95 ms) +31.8ms (1.8%) 👎
tablecmp-create-1k metric base(c2ca844) target(e0e5f9e) trend
benchmark-table-component/create/1k duration 195.40 (±5.50 ms) 201.00 (±4.70 ms) +5.6ms (2.9%) 👎
tablecmp-update-10th-1k metric base(c2ca844) target(e0e5f9e) trend
benchmark-table-component/update-10th/1k duration 71.50 (±5.55 ms) 73.10 (±4.45 ms) +1.6ms (2.2%) 👌
wc-append-1k metric base(c2ca844) target(e0e5f9e) trend
benchmark-table-wc/append/1k duration 268.65 (±9.75 ms) 284.25 (±12.00 ms) +15.6ms (5.8%) 👎
wc-clear-1k metric base(c2ca844) target(e0e5f9e) trend
benchmark-table-wc/clear/1k duration 13.15 (±2.25 ms) 13.45 (±1.65 ms) +0.3ms (2.3%) 👌
wc-create-10k metric base(c2ca844) target(e0e5f9e) trend
benchmark-table-wc/create/10k duration 2145.40 (±21.50 ms) 2214.70 (±17.50 ms) +69.3ms (3.2%) 👎
wc-create-1k metric base(c2ca844) target(e0e5f9e) trend
benchmark-table-wc/create/1k duration 257.45 (±4.50 ms) 259.15 (±4.90 ms) +1.7ms (0.7%) 👎
wc-update-10th-1k metric base(c2ca844) target(e0e5f9e) trend
benchmark-table-wc/update-10th/1k duration 73.45 (±6.75 ms) 71.00 (±5.15 ms) -2.4ms (3.3%) 👌

@pmdartus pmdartus requested review from caridy and removed request for caridy June 12, 2019 09:45
@pmdartus pmdartus requested a review from diervo June 12, 2019 09:45
@caridy
Copy link
Contributor

caridy commented Jun 12, 2019

I know that @diervo and @caridy will have some strong opinion here =)

I have no opinions here! what a surprise! jajaja! LGTM

@diervo
Copy link
Contributor

diervo commented Jun 12, 2019

I'm ok with this so long I can blame @pmdartus next time we break due to ^ 🎉

@ekashida ekashida merged commit 55ec829 into master Jun 13, 2019
@ekashida ekashida deleted the ekashida/dependencies branch June 13, 2019 03:05
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