Skip to content

[ENG-467] Upgrade dependencies #654

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

Merged

Conversation

jamescdavis
Copy link
Member

@jamescdavis jamescdavis commented Apr 26, 2019

Purpose

Upgrade most dependencies to latest stable versions in preparation for Ember version upgrade.

Summary of Changes

  • upgrade all dependencies with latest patch versions
  • upgrade all dependencies with latest minor versions except:
    • ember-source
    • ember-data
    • ember-cli
    • ember-cli-meta-tags (some change in 5.1.0 causes things to break in weird ways)
  • upgrade most other dependencies to latest major version, making any code changes needed
    not upgraded:
    • ember-bootstrap (too many things break in 2.x)
    • ember-cli-addon-docs (custom fork)
    • ember-cli-addon-docs-typedoc (custom fork)
    • ember-cli-babel (7.x causes builds to fail)
    • ember-cli-clipboard (pinned due to some incompatibility)
    • ember-cli-template-lint
    • ember-cli-typescript
    • ember-content-placeholders (custom fork)
    • ember-decorators
    • ember-engines (custom fork)
    • ember-font-awesome
    • ember-metrics (custom fork)
    • ember-parachute (custom fork)
    • keen-analysis
    • keen-dataviz
    • keen-tracking
    • stylelint-config-sass-guidelines
    • tsutils

Side Effects

hopefully not

Feature Flags

n/a

QA Notes

This does not require any specific QA.

Ticket

https://openscience.atlassian.net/browse/ENG-467

Reviewer Checklist

  • meets requirements
  • easy to understand
  • DRY
  • testable and includes test(s)
  • changes described in CHANGELOG.md

@coveralls
Copy link

coveralls commented Apr 26, 2019

Pull Request Test Coverage Report for Build 2934

  • 37 of 40 (92.5%) changed or added relevant lines in 14 files are covered.
  • 10 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.4%) to 71.97%

Changes Missing Coverage Covered Lines Changed/Added Lines %
app/guid-user/quickfiles/controller.ts 0 1 0.0%
app/models/osf-model.ts 0 1 0.0%
lib/osf-components/addon/components/editable-field/tags-manager/component.ts 2 3 66.67%
Files with Coverage Reduction New Missed Lines %
lib/osf-components/addon/components/validated-input/checkboxes/component.ts 1 60.0%
lib/osf-components/addon/components/validated-input/power-select/component.ts 1 80.43%
lib/osf-components/addon/components/validated-input/date/component.ts 1 77.42%
lib/osf-components/addon/components/editable-field/institutions-manager/component.ts 1 87.67%
lib/osf-components/addon/components/validated-input/recaptcha/component.ts 2 74.42%
lib/osf-components/addon/components/editable-field/tags-manager/component.ts 4 79.31%
Totals Coverage Status
Change from base Build 2933: 0.4%
Covered Lines: 6146
Relevant Lines: 7694

💛 - Coveralls

@jamescdavis jamescdavis force-pushed the upgrade_dependencies branch 4 times, most recently from ac7dc06 to b3e816c Compare April 29, 2019 19:20
Copy link
Member Author

@jamescdavis jamescdavis left a comment

Choose a reason for hiding this comment

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

Here's some comments explaining why individual changes were necessary.

}),
column.set('subjects', taxonomies ? taxonomies.toArray() : []);
},
),
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 reformatted this, but the only change is adding types for parents and tier.

@@ -291,6 +291,9 @@ module.exports = function(environment) {
mirageScenarios: MIRAGE_SCENARIOS,

defaultProvider: 'osf',
pageTitle: {
prepend: false,
},
Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -94,7 +94,7 @@ export default class OsfModel extends Model {

// HACK: ember-data discards/ignores the link if an object on the belongsTo side
// came first. In that case, grab the link where we expect it from OSF's API
const url = reference.link() || getRelatedHref(this.relationshipLinks[relationshipName]);
const url = reference.link() || getRelatedHref(this.relationshipLinks[relationshipName as string]);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is due to the types changes to RelationshipsFor. RelationshipsFor includes the possibility of attribute keys of type symbol, which type Relationships does not accept. We previously restricted to string keys:

export type RelationshipsFor<Model, T extends keyof Model = keyof Model> = T extends string ? T : never;

but this now breaks this.hasMany() above due to stricter typing in ember-data.

@@ -156,7 +156,7 @@
background: url('/assets/images/home/Ana_transparent_gradient.png') no-repeat center bottom;
background-size: contain;
position: relative;
margin: -80px 0 -65px 0;
margin: -80px 0 -65px;
Copy link
Member Author

@jamescdavis jamescdavis Apr 29, 2019

Choose a reason for hiding this comment

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

@@ -44,7 +44,7 @@ export default class UserQuickfiles extends Controller {
}
});

flash = task(function *(item: File, message: string, type = 'success', duration = 2000) {
flash = task(function *(item: File, message: string, type: string = 'success', duration: number = 2000) {
Copy link
Member Author

@jamescdavis jamescdavis Apr 29, 2019

Choose a reason for hiding this comment

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

There appears to be a change (possibly a regression) in TypeScript 3.4 that prevents it form inferring the type from default arguments in some cases (turn on the noImplicitAny option). Here's the same with TypeScript 3.3 (as of this writing typescript-play.js.org was still on 3.3. Note: noImplicitAny is on by default in this playground).

@@ -17,7 +21,7 @@ module('Unit | Metrics Adapter | keen ', hooks => {
const trackPublic = this.stub(adapter, 'trackPublicEvent');
const trackPrivate = this.stub(adapter, 'trackPrivateEvent');

this.stub(adapter, 'getCurrentNode').resolves({ public: true });
this.stub(adapter, 'getCurrentNode').resolves(make('node', { public: true }) as Node);
Copy link
Member Author

Choose a reason for hiding this comment

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

Stricter Sinon types need a Node here, so we make one.

@@ -216,7 +217,8 @@ module('Unit | Model | osf-model', hooks => {
const pageSize = 100;

for (const total of totals) {
let allResults = Array.from({ length: total }).map(() => ({}));
const id = 1;
let allResults = Array.from({ length: total }).map(() => ({ id, modelName: 'foo' }));
Copy link
Member Author

Choose a reason for hiding this comment

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

Sinon stub return type needs to match sparseHasMany.

export type RelationshipsFor<Model, T extends keyof Model = keyof Model> = T extends string ? T : never;
export type ModelKeys<Model extends DS.Model> = Exclude<keyof Model, keyof DS.Model>;
export type AttributesFor<Model extends DS.Model> = ModelKeys<Model>;
export type RelationshipsFor<Model extends DS.Model> = ModelKeys<Model>;
Copy link
Member Author

Choose a reason for hiding this comment

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

Match what's in the ember-data types. (these are duplicated here only because they're not exported from ember-data)

@@ -17,7 +17,7 @@ module('Unit | Validator | password-strength', hooks => {
const validator = this.owner.lookup('validator:password-strength');
const options = { min: i };
const builtOptions = validator.buildOptions(options);
const message = await validator.validate(minima[i], builtOptions.copy());
const message = await validator.validate(minima[i], Object.assign({}, builtOptions));
Copy link
Member Author

Choose a reason for hiding this comment

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

.copy() is removed in newer versions of ember-validator.

});
const user = FactoryGuy.make('user');
user.set('links', { self: url });

Copy link
Member Author

@jamescdavis jamescdavis Apr 29, 2019

Choose a reason for hiding this comment

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

ember-data-factory-guy 3.2.1 makes some breaking changes to links.

@jamescdavis jamescdavis force-pushed the upgrade_dependencies branch from b3e816c to 1580171 Compare May 3, 2019 20:02
@@ -76,7 +76,7 @@ export default Factory.extend<MirageNode & NodeTraits>({

withContributors: trait<MirageNode>({
afterCreate(node, server) {
const contributorCount = faker.random.number({ min: 1, max: 25 });
const contributorCount = faker.random.number({ min: 1, max: 5 });
Copy link
Member Author

Choose a reason for hiding this comment

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

25 make mirage too slow.

@jamescdavis jamescdavis requested a review from aaxelb May 6, 2019 17:07
@jamescdavis jamescdavis changed the title Upgrade dependencies [ENG-275] Upgrade dependencies May 7, 2019
@jamescdavis jamescdavis changed the title [ENG-275] Upgrade dependencies [ENG-467] Upgrade dependencies May 7, 2019
aaxelb and others added 2 commits May 20, 2019 11:58
They should respond properly to changes in user permissions, even though
it will rarely matter outside of tests.
@jamescdavis jamescdavis merged commit 0bd7503 into CenterForOpenScience:develop May 20, 2019
@jamescdavis jamescdavis deleted the upgrade_dependencies branch May 20, 2019 20:02
@jamescdavis jamescdavis added this to the 19.5.0 milestone May 30, 2019
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.

3 participants