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

Recognize .tsx source as Typescript #16944

Merged
merged 1 commit into from Mar 13, 2018

Conversation

Projects
None yet
3 participants
@smashwilson
Member

smashwilson commented Mar 13, 2018

Requirements

Description of the Change

Extra kudos to @kuychaco, who did the (rather painful) work of tracking this down yesterday.

Ensure that .tsx is added to require.extensions by the CompilerCache.

Although the (ancient) version of Typescript that we bundle doesn't understand .tsx files itself, this will allow packages using atom-ts-transpiler to properly transpile .tsx source with newer Typescript releases.

Alternate Designs

Alas, importing modules with a specified extension is explicitly disallowed by Typescript itself: see Microsoft/TypeScript#11901.

In general, it may be nice to allow custom transpilers to contribute recognized extensions to be added to require.extensions in addition to the built-in ones from COMPILERS. Personally, I'd wait until we had at least one more transpiler that needs this before adding that complexity, though.

Why Should This Be In Core?

atom-ts-transpiler cannot import .tsx files until this change is in place.

Benefits

TypeScript packages will be able to use .tsx source files and use React or Etch properly.

Possible Drawbacks

If any existing packages are using the built-in TypeScript transpiler and contain .tsx source, they'll break because the old TypeScript version doesn't actually support them. But, .tsx source wouldn't have been working for them in the first place, and this would be a nudge to transition to a custom transpiler anyway.

Verification Process

I built a minimal TypeScript and React package and loaded it in dev mode with and without this change. Without it, the import statement for the .tsx file fails; with it, the package works correctly.

/cc @kuychaco @daviwil This should unblock our path to using TypeScript 🙌

@daviwil

Looks great to me! Thanks a bunch to @kuychaco for going through the pain of trying this out :)

@smashwilson smashwilson merged commit 87b77a7 into master Mar 13, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@smashwilson smashwilson deleted the aw-tsx branch Mar 13, 2018

daviwil added a commit that referenced this pull request Mar 13, 2018

Merge pull request #16944 from atom/aw-tsx
Recognize .tsx source as Typescript
@thomasjo

This comment has been minimized.

Member

thomasjo commented Mar 14, 2018

FYI, this was resolved in #16209 back in November 2017.

@thomasjo

This comment has been minimized.

Member

thomasjo commented Mar 14, 2018

Also note that this PR will not work for anyone who does not explicitly use a more modern TypeScript transpiler, since the version we're bundling has no support for .tsx files. At the very least we should document that problem somewhere obvious.

@smashwilson

This comment has been minimized.

Member

smashwilson commented Mar 14, 2018

FYI, this was resolved in #16209 back in November 2017.

Personally, I'm 👎 on upgrading the bundled versions of TypeScript (or Babel, CoffeeScript, or Jasmine, or any of the other dependencies we bundle for package author consumption) for anything other than high priority backwards-compatible, security-related changes. In general it's too risky to force upgrades on the entire package ecosystem like that. I see the immediate way forward as encouraging the use of custom transpilers that allow each package to keep up with dependencies at their own pace.

As a sidenote, for the non-immediate way forward, I'd love to see Atom get out of the business of transpiling altogether by making apm publish transpiled artifacts. That's even more engineering effort though, and I have no idea how soon we'd be able to start revisiting apm's design.

Also note that this PR will not work for anyone who does not explicitly use a more modern TypeScript transpiler, since the version we're bundling has no support for .tsx files. At the very least we should document that problem somewhere obvious.

My logic is that any packages using the bundled TypeScript transpiler wouldn't have preexisting .tsx files in them because the bundled TypeScript transpiler doesn't support them. This change will only make the error slightly different, not introduce a new one.

Good point about documentation. I'll do a pass through the flight manual and docs/ to see what I can update re: writing TypeScript packages.

  • Find all community packages that use our built-in TypeScript support.
    • Check if any of these packages are affected by breaking changes.
    • Submit PRs to affected packages.
  • Celebrate.

I'd be really curious to see how large of a population that set is ☝️ and to see the install base of those packages is. Maybe we could even yank the built-in TypeScript transpiler altogether, and give custom transpilers a mechanism to specify require extensions instead?

I know @maxbrunsfeld has a script to do operations on every published package... I've asked him what it is like three times, but I can never remember it 😅

@thomasjo

This comment has been minimized.

Member

thomasjo commented Mar 14, 2018

I'd be really curious to see how large of a population that set is ☝️ and to see the install base of those packages is. Maybe we could even yank the built-in TypeScript transpiler altogether, and give custom transpilers a mechanism to specify require extensions instead?

I checked sometime in December 2017, but I never got around to actually opening PRs, etc. If I remember correctly, less than 10 packages in total would possibly be affected by the upgrade. Verifying what, if anything broke, would require manual testing on a few packages due to missing or limited automated tests.

For the record, I completely agree with not upgrading bundled transpilers, but only if that means we are planning on removing all bundled transpilers. It's better to not ship anything, than to ship old, outdated bits (with known performance issues, and what not).

@smashwilson smashwilson referenced this pull request Mar 20, 2018

Open

Remove the bundled TypeScript transpiler #17001

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment