Skip to content
This repository has been archived by the owner on Mar 12, 2020. It is now read-only.

[Toolchain] Update Typescript to 3.0.3 #1185

Merged
merged 4 commits into from
Sep 12, 2018
Merged

Conversation

damassi
Copy link
Member

@damassi damassi commented Sep 11, 2018

Upgrades Typescript and TSLint to latest, and followed a few work-arounds to get it green since we're a few versions behind in terms of react-native / react.

@@ -5,6 +5,7 @@
"baseUrl": "src",
"experimentalDecorators": true,
"jsx": "react",
"keyofStringsOnly": true,
Copy link
Member Author

@damassi damassi Sep 11, 2018

Choose a reason for hiding this comment

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

Copy link
Contributor

@alloy alloy left a comment

Choose a reason for hiding this comment

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

Are all the Relay artefact file changes related to previously running prettier against them?


-export var Geolocation: GeolocationStatic;
-export type Geolocation = GeolocationStatic;
+// https://github.com/DefinitelyTyped/DefinitelyTyped/issues/24573
Copy link
Contributor

Choose a reason for hiding this comment

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

Gah, didn’t know we had this issue. This should really be worked around by getting rid of @types/node, because RN is not Node.

Looks like it’s being pulled in by enzyme/cheerio?? No clue why that would be a good idea of them.

=> Found "@types/node@8.5.1"
info Reasons this module exists
   - "enzyme#cheerio#parse5" depends on it
   - Hoisted from "enzyme#cheerio#parse5#@types#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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually meant that no package should list @types/* packages as runtime deps. In this case it was parse5 (didn’t read properly yesterday). You can see that in v5.1.0 they removed this inikulin/parse5@4350f6a#diff-b9cfc7f2cdf78a7f4b91a753d10865a2, so the fix would be for us to force resolution of that version instead of the one that cheerio depends on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting:

screen shot 2018-09-12 at 12 37 59 pm

I removed the patch, and updated the resolution, yet the error re-appears.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do yarn why @types/node instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

screen shot 2018-09-12 at 12 43 39 pm

@damassi
Copy link
Member Author

damassi commented Sep 11, 2018

@alloy - Oh weird, I wonder what happened there re relay fragments; my first push only sent up a couple files. I think this might have just happened on rebase -- investigating.

@damassi
Copy link
Member Author

damassi commented Sep 11, 2018

Yup, this looks like a Prettier thing which I presume was triggered after Typescript re-ran post update.

"member-access": [false, "check-accessor", "check-constructor"],
"no-console": [true, ["error", ["warn", "error"]]],
"no-unused-variable": [true, { "ignore-pattern": "^_" }],
Copy link
Member Author

Choose a reason for hiding this comment

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

@artsy artsy deleted a comment from peril-staging bot Sep 11, 2018
@orta
Copy link
Contributor

orta commented Sep 12, 2018

Part of me wonders if we should add **/__generated__/*.graphql.ts to the prettier ignore? Otherwise feels pretty fine to me - it supports globs

@damassi
Copy link
Member Author

damassi commented Sep 12, 2018

@orta - I would agree with this; I don't think we should transform generated files.

@alloy
Copy link
Contributor

alloy commented Sep 12, 2018

This LGTM. It would be great if you could address that @types/node issue, but not a necessity.

@alloy
Copy link
Contributor

alloy commented Sep 12, 2018

@damassi Sigh. Well, commit that anyways, there’s absolutely no need for @types/node in Emission. Then restore the RN typedef patch and go ahead with the merge 👍

@damassi
Copy link
Member Author

damassi commented Sep 12, 2018

Ok mergin'

@damassi damassi merged commit 8c61793 into master Sep 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants