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 to TS 2.1 #492

Merged
merged 10 commits into from
Jan 24, 2017
Merged

Upgrade to TS 2.1 #492

merged 10 commits into from
Jan 24, 2017

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented Jan 16, 2017

Fixes #376, Fixes #11

Checklist

Changes proposed in this pull request:

  • update typescript and gulp-typescript
  • replace Object.assign and Utils.shallowClone with object spreads
  • some lint fixes in table examples
  • ⚠️ new dependency on tslib for --importHelpers flag
    • this reduces the size of each source file at the cost of one additional NPM dependency
  • use Partial<Props> in test suites for components that have required props!
  • use extends in tsconfig.json files that are never built with gulp-typescript

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

the "extends" thing is really unfortunate... I've started using it in other projects and am a big fan

@@ -50,17 +50,6 @@ export function clamp(val: number, min: number, max: number) {
return Math.min(Math.max(val, min), max);
}

/** Return a new object with the same keys as the given object (values are copied, not cloned). */
export function shallowClone<T>(object: T): T {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

@@ -40,51 +40,52 @@ import {
Utils,
} from "../src/index";

// tslint:disable:max-classes-per-file no-console jsx-no-lambda jsx-no-multiline-js
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to disable jsx-no-multiline-js? would prefer to keep it

Copy link
Contributor Author

@giladgray giladgray Jan 16, 2017

Choose a reason for hiding this comment

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

same but would prefer not to touch this file too much. most of what you see here was done automatically by TSLint or VSCode formatting.

isColumnResizable: false,
isRowResizable: false,
selectionModes: SelectionModes.NONE,
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

this indentation change looks wrong, the previous alignment is preferred

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, i didn't look closely at the formatting. maybe i'll just revert, it's not super relevant.

],
},
],
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@adidahiya
Copy link
Contributor

what do you plan to do with tslib and Partial?

@giladgray
Copy link
Contributor Author

giladgray commented Jan 16, 2017

avail myself of them :) marking this WIP for now.

hence the unchecked tasks.

@giladgray
Copy link
Contributor Author

FWIW, i'm able to use extends in a few places, like test tsconfigs, which are only ran via webpack. might be able to do examples too for the same reason.

so it really only affects packages/*/tsconfig.json, the ones that get run through gulp-typescript.

Gilad Gray added 2 commits January 16, 2017 18:54
- add `tslib` dependency
- test tsconfigs can safely extend package root tsconfigs because tests are only run from webpack (never gulp-typescript)
@giladgray
Copy link
Contributor Author

@themadcreator @gscshoyru Table code coverage is failing the build. seems like a test for column resizing would fix it swimmingly. might one of you be able to take that one this week?

https://1214-71939872-gh.circle-artifacts.com/0/home/ubuntu/blueprint/packages/table/coverage/PhantomJS%202.1.1%20(Linux%200.0.0)/src/table.tsx.html

ideal for components that have required props!
@adidahiya
Copy link
Contributor

@giladgray why would coverage change as a result of this PR? Those things should really be decoupled; please don't add a test in this PR.

@giladgray
Copy link
Contributor Author

giladgray commented Jan 17, 2017

i'm not planning to add a test in this PR and I'm not sure exactly why coverage changed. you'll notice it's extremely close to the threshold (79%, must be 80% to pass) so all it would have taken is one little tweak to drop coverage below the threshold. my guess is that switching to tslib removed enough code from the file as to reveal this.

@blueprint-bot
Copy link

reduce code coverage limit by 1% to fix build

Preview: docs | landing | table
Coverage: core | datetime

@giladgray
Copy link
Contributor Author

ready 4 review 👀

@adidahiya adidahiya self-assigned this Jan 23, 2017
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

overall, looks good, but:

this reduces the size of each source file at the cost of one additional NPM dependency

sorry, performance enhancements shouldn't be merged without measurements. what is the impact of this for the end user? are they actually saving on bundle size?

@adidahiya
Copy link
Contributor

nvm, I read the 2.1 release notes, this looks legit

@adidahiya adidahiya merged commit 53a0721 into master Jan 24, 2017
@adidahiya adidahiya deleted the gg/ts21 branch January 24, 2017 17:36
@giladgray giladgray mentioned this pull request Jan 27, 2017
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants