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

Run typescript sources through full babel transpiling pipeline #3369

Merged
merged 3 commits into from
Jan 25, 2018

Conversation

farcaller
Copy link
Contributor

@farcaller farcaller commented Dec 30, 2017

This partially fixes #3356, maybe #3274 (#3274 fails to be minified).

Open issues:

  • tests fail. I have no idea how to mock program for tests, as it comes from gatsby-cli and I'm not familiar enough with the test framework to try mocking that out.
  • async/await syntax works now (as it goes through babel as expected), but fails to be minified.
  • UglifyJsPlugin.js still fails to bubble up the error / break the build with an error.
  • transpileOnly seems to be broken badly and is only used in tests.

@gatsbybot
Copy link
Collaborator

gatsbybot commented Dec 30, 2017

Deploy preview ready!

Built with commit 9477ff8

https://deploy-preview-3369--gatsbygram.netlify.com

@jquense
Copy link
Contributor

jquense commented Dec 30, 2017

Thanks for the PR! I think we'd really like to avoid full Babel processing in addition to running through the typescript compiler, since it's a lot of potentially extra work. In the case of async/await that should work with just the TS compiler I think we'd want to figure out why that isn't working before adding an additional solution

@farcaller
Copy link
Contributor Author

@jquense I don't think that's possible. To make TS emit safe code you need to target es5, which would do things like turning arrow functions into "old-style" functions. But that also wipes away the tagged templates (which are es6) and babel-plugin-extract-graphql can no longer visit them as they are not tagged template expressions but are rendered into function calls (as confirmed by WebpackError: graphql is not defined).

@farcaller
Copy link
Contributor Author

I'll hijack this PR to list (and hopefully fix) everything that's broken in gatsby in re. typescript support if you don't mind?

Re. #3274,

@wedelgaard I've figured what's wrong with async/await and this PR won't fix them completely. Apparently babel transpiles async/await syntax into generators (that's es6 too) and uglify chokes up on those then. One way to fix it would be by using transform-regenerator in the babel pipeline and adding the regenerator runtime, but I'm not sure how polyfills work with all these webpack-generated modules.

Re #3356,

I've found the place in gatsby that mutes webpack errors and propagated them properly, so all the issues raised in #3356 are fixed now.

Re transpileOnly,

I've misread the docs, apparently what it does is that it disables everything in typescript that compiles and verifies the code. So as of now the default version of gatsby-plugin-typescript is totally useless as it will ignore any ts errors. I guess the idea was that VSCode or any other ts-capable editor would do the magic? That's not the right way to do typescript though...

As for switching transpileOnly to true, that causes a whole storm of errors within react. Insofar I found yarnpkg/yarn#1334 that might explain the problems within yarn. Indeed, npm install doesn't break TS right there (it breaks it in other places). I've searched down to a random comment here: aspnet/JavaScriptServices#1132 (comment), reverted the types to versions noted and finally make ts compile the code, but this solution is kinda subpar.

@farcaller
Copy link
Contributor Author

After poking around yarn more I figured that the version issue is covered by yarnpkg/rfcs#68, thus the typescript starter can be fixed with:

  "dependencies": {
    "@types/react": "^15.6.2", // existing dependency
    ...
  },
  "resolutions": {
    "**/@types/react": "^15.6.2"
  },

by forcing all the types to the same version.

@farcaller
Copy link
Contributor Author

It seems that transform-regenerator doesn't have any negative side-effects on the build overall, but I'm hesitant to include it into this PR as it requires runtime support. I'd suggest the following as a workaround to fix #3274:

Add .babelrc having

{
  "plugins": ["transform-regenerator"]
}

Import polyfill that implements generators in index.jsx:

import 'babel-polyfill';

I've verified that it's enough to make async/await functional in runtime

@farcaller
Copy link
Contributor Author

@jquense, @KyleAMathews anything else you want me to do so this could land? Typescript support is still broken.

I can work on moving the pipeline over to awesome-typescript-loader that has a native support for babel (and caching), but I'd prefer to make the current code functional first.

@jquense
Copy link
Contributor

jquense commented Jan 15, 2018

To make TS emit safe code you need to target es5, which would do things like turning arrow functions into "old-style" functions. But that also wipes away the tagged templates (which are es6) and babel-plugin-extract-graphql can no longer visit them

yes bable with JUST this transform should be run before typescript, which is waht is happening now. then TS compiler is left to compile whatever is left

@farcaller
Copy link
Contributor Author

farcaller commented Jan 15, 2018

@jquense webpack loaders are applied right-to-left, so it's first transpiled by TS, then babel runs its pass.

@farcaller
Copy link
Contributor Author

... and if I swap the order to run babel first and then the ts pass, then babel (expectedly) chokes up on the typescript code.

@jquense
Copy link
Contributor

jquense commented Jan 15, 2018

I didn't notice the direction of the loaders sorry :P

I still not sure though how this addresses the problem. Either you run the source through babel first and it will choke on the TS or after TS and it will fail because the tagged templates are gone. It seems like the "correct" way to to fix this is to build a TS equivalent to the babel plugin here (using the ts compiler). This should be feasible? you don't need real code generation, just the location of the tags and a string replace after they are extracted. The other potential option is babel 7 which does have TS parsing support

@farcaller
Copy link
Contributor Author

I think the best route is to use something akin to the awesome-typescript-loader that is specifically tailored for TS+Babel pipelines and let it do the caching.

Rationale: let TS emit the richest possible output (esnext in this case) and then run it through exactly the same pipeline that exists now. Yes, it might be marginally slower, but it's the most stable approach that's not going to break randomly and will be flexible to support any changes in the global babel pipeline that's managed by gatsby.

This has an added benefit of having to deal only with babel polyfills as TS won't generate any library code in this case.

config.loader(`typescript`, {
test,
loaders: [
`babel?${JSON.stringify({ plugins: [extractQueryPlugin] })}`,
`babel?${JSON.stringify(babelConfig)}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

rather then passing through the babel config can you stick the ts loader in front of the existing js loader in the chain. You can also "override" the js loader and pass a function the gets the current one and alters it.

its not clean, but this will be much easier to do neatly in v2, and i don't think we want to add more surface area if we can avoid it, since its much harder to deprecate later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I do that then typescript becomes a hard dependency. I don't think that makes sense for most people using gatsby.

@KyleAMathews
Copy link
Contributor

@farcaller can you profile this change a bit to see roughly how much slower this PR makes things? If it's not too much, I think it'd be fine. We're moving to Babel 7 with Gatsby v2 which will give us more options.

@farcaller
Copy link
Contributor Author

@farcaller can you profile this change a bit to see roughly how much slower this PR makes things

@KyleAMathews it is marginally slower, but don't forget that I'm comparing a fast and totally broken config with a slower and actually functional one. I don't think this should be a case where a faster option wins—the current plugin doesn't do what it advertises and breaks the prod builds.

@KyleAMathews
Copy link
Contributor

I'm comparing a fast and totally broken config with a slower and actually functional one.

Ok, sold :-)

Better working than fast. Make it work, make it correct, make it fast. Let's revisit this in Gatsby v2 once we're on webpack 3.

@jurosh
Copy link
Contributor

jurosh commented Jan 31, 2018

Thanks a lot guys! Those TypeScript issues were pretty serious and just now I updated Gatsby and looks like minification works fine..
ps. I would be also fan of removing babel when doing compilation with TypeScript (to reduce build complexity) - already tried to do it, but that extraction of queries is blocker here as tsc compiler doesn't allow usage of plugins like we can use in babel...

@jlengstorf
Copy link
Contributor

Hiya @farcaller! 👋

This is definitely late, but on behalf of the entire Gatsby community, I wanted to say thank you for being here.

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (We’ve got t-shirts and hats, plus some socks that are really razzing our berries right now.)
  2. If you’re not already part of it, we just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. You’ll receive an email shortly asking you to confirm. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If you have questions, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again! 💪💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gatsby build fails to minify js if typescript is used, doesn't show the error
6 participants