Enable Typescript 3.8 and update Sewing-kit to latest #2873
Conversation
BPScott
changed the title
| DataTableProps, | ||
| TableData, | ||
| TableRow, | ||
| SortDirection, | ||
| ColumnContentType, | ||
| } from './DataTable'; | ||
|
|
||
| export {DatePicker, DatePickerProps, Range, Months, Year} from './DatePicker'; | ||
| export {DatePicker, Months} from './DatePicker'; |
There was a problem hiding this comment.
I'm new to this, and find surprising that Months is in this export statement (rather than the one below, alongside type exports), being an enum. Can you explain why this is the case?
There was a problem hiding this comment.
Broadly speaking you've got two sorts of thing: "values" which have some kind of JS output, and "types" which are only used at compile time and get stripped away from the resultant JS.
A enum ends up outputting javascript code so it is a value (same as something like const a =1), which means it gets exported with all our other values (like constants and functions etc).
|
Suggestion: relaxing or disabling |
This has some rather funky knock on effects as it intersects with a handful of other rules around selector specificity - you'd want to update them all at the same time. I don't think it's particularly worth loosening the rules as we mostly fall afoul of this because we're wrapping stuff in |
| @@ -22,7 +22,8 @@ import {monthName} from './utilities'; | |||
| import {Month} from './components'; | |||
| import styles from './DatePicker.scss'; | |||
|
|
|||
| export {Range, Months, Year}; | |||
| export {Months}; | |||
There was a problem hiding this comment.
What's going on here? Is Months not a type even though it's imported from the same place as Year and Range?
There was a problem hiding this comment.
Bingo. Months is an enum, while Range and Year are both types. See https://github.com/Shopify/javascript-utilities/blob/master/src/dates.ts#L11
Follow up questions: is this annoying and would it be better if Months was a union type like type Months = 0 |1|2|3|4|5|6|7|8|9|10|11? Yeah probably, but thar be dragons in the javascript-utilities packages and I want to stop relying on it eventually so just stick your fingers in your ears and ignore it for now :D
One drawback I can see is that this is a large PR that touches a lot of files, meaning the risk of encountering merge conflicts increases quite a bit as time goes by. Not a huge issue, but could be leading to more work than shipping this in 2 steps. |
Yep, that pretty much. Getting this in now unblocks me doing other stuff, that'll be much easier to shuffle around without all this other noise. The update to the currently-unreleased SK will comparativly empty as there won't be any new linting rules appearing, that'll be less effort than trying to rebase this lot. |
WHY are these changes introduced?
Typescript 3.8 contains new syntax abilities (
export type) that enable my machinations for drastically simplifying our build process - moving from a two-step thing where we compile our TS files with typescript, then compile that output with babel, to having babel reading the TS files directly.It's not as simple as "just update typescript" though, as we have to also update all our linting tooling to new versions that understand this new syntax too, which is why we have to update sewing-kit too.
A sewing-kit update includes updates to our linting rules which are almost entirely autofixable but make this PR look a lot bigger and scarier than what it actually is.
Ben, why are you giving us ~400 file PRs to review on hackdays FFS
Yeah sorry. I figure it's a lot cleaner to do this piece by piece, get this pulled into the v5 branch, and then make do my build wrangling there, rather than hacking together an even bigger monstrosity that I then have to pick apart and submit piece by piece later on anyway.
WHAT is this pull request doing?
export {SomeType}, you have to doexport type {SomeType}.How to 🎩
It's probably best to code review this on a per-commit basis and you can skip over the "automated update from sk format" commits as that's the same sort of whitespace changes in every file.
Ensure linting, tests and typechecks all pass and storybook looks the same as before.
I've compared the build output of the esnext folder, our index.js, index.es.js and styles.css files with the output from master and they're all functionally identical - just a few changes from babel updates in the index.js and index.es.js files.