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

Add ios type-checking #491

Merged
merged 1 commit into from Jun 4, 2019
Merged

Add ios type-checking #491

merged 1 commit into from Jun 4, 2019

Conversation

zephraph
Copy link
Contributor

@zephraph zephraph commented Jun 3, 2019

Figured out how to get a type-checking task working for iOS!!

@artsyit
Copy link
Contributor

artsyit commented Jun 3, 2019

Deploy preview for artsy-palette ready!

Built with commit eea9c24

https://deploy-preview-491--artsy-palette.netlify.com

"baseUrl": ".",
"paths": {
"./*": ["./*.ios", "./*"],
"../*": ["../*.ios", "../*"]
Copy link
Member

Choose a reason for hiding this comment

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

What's this one for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meannn... ./ and ../ are different paths technically.

Copy link
Member

@damassi damassi left a comment

Choose a reason for hiding this comment

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

Nice and simple 👍 Will wait for @alloy and / or @ashfurrow to approve before merging tho. (Also want to add microsoft/TypeScript#8328 as a reference per previous slack discussions.)

"compilerOptions": {
"baseUrl": ".",
"paths": {
"./*": ["./*.ios", "./*"],
Copy link
Contributor

Choose a reason for hiding this comment

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

:ceiling-cat:

Can I suggest adding a link to microsoft/TypeScript#8328 (comment) in a comment here? This is a fairly advanced bit of config and it might save someone some googling one day.

:ceiling-cat-out:

@alloy
Copy link
Contributor

alloy commented Jun 4, 2019

Niiice!

I recall there not being a solution last time around, but that can’t have been in 2016… so not sure why we didn’t go with this last time around 🤔 Anyways, glad that we have this now 👍

I agree with @ds300 about leaving a comment. Not sure if tsconfig is json5, if not you should add that context to the commit message.

@zephraph
Copy link
Contributor Author

zephraph commented Jun 4, 2019

Can't really do a comment (it's not json5), but I'll update the commit message.

See this comment for the initial inspiration: microsoft/TypeScript#8328 (comment)

This change adds a new ts config file that overwrites it's module resolution behavior. When importing a relative file
it first tries to find a *.ios version of the file. This is in line with how react-native handles platform specific implementations.
@zephraph
Copy link
Contributor Author

zephraph commented Jun 4, 2019

Okay @ds300, @alloy I edited the commit to be much more detailed w/ some context.

@ds300
Copy link
Contributor

ds300 commented Jun 4, 2019

Thanks 🙏

Btw, pretty sure tsconfig files are json5! At least tsc treats them as such and has done for years so most TS tooling should have caught up.

@damassi damassi merged commit b1ef459 into master Jun 4, 2019
@alloy alloy deleted the ios-type-check branch June 4, 2019 18:24
@artsyit
Copy link
Contributor

artsyit commented Jun 5, 2019

🚀 PR was released in v4.14.4 🚀

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.

None yet

5 participants