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

Switch from ts-loader to babel-loader #2449

Merged
merged 5 commits into from Apr 19, 2020
Merged

Conversation

gopeter
Copy link
Contributor

@gopeter gopeter commented Feb 6, 2020

Using ts-loader has some drawbacks:

  • We can't use babel-macros with ts-loader (but it's already included in rails/webpacker and therefore should work)
  • To make presets like @babel/preset-env work, the ts-loader has to be combined with babel-loader, which means that we have to configure and manage multiple loaders (which may decrease build times)

I'm not sure if it's good to add package/rules/typescript.js since it has the same content as package/rules/babel.js. We could add .ts and .tsx directly to the babel.js loader, but then Webpacker would be able to compile .ts and .tsx files without running webpacker:install:typescript before, which is essential to make it work correctly.

Anyway, I'm not sure if the switch away from ts-loader is going to be merged, but with the current setup I can't use babel-macros together with TypeScript (like styled-components/macro and graphql.macro).

@gopeter
Copy link
Contributor Author

gopeter commented Feb 6, 2020

The builds are failing due to connection issues 😩

image

@gopeter
Copy link
Contributor Author

gopeter commented Apr 8, 2020

I don't know who to address this to, so I try @gauravtiwari and @jakeNiemiec :-) Could this request be of interest? Otherwise I would close this and just continue to maintain my fork.

@gauravtiwari
Copy link
Member

Thanks @gopeter Apologies for the delay. Yes, definitely please keep it open.

Will take a look at all open PRs this weekend.

// Process application TypeScript code with Babel.
// Uses application .babelrc to apply any transformations
module.exports = {
test: /\.(ts|tsx)?$/,
Copy link
Member

Choose a reason for hiding this comment

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

@gopeter Can we use the same babel loader to enable typescript as well?
https://github.com/rails/webpacker/blob/master/package/rules/babel.js#L8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be possible if we add the ts/tsx extensions to it. But then Babel would be able to parse Typescript files, even without running webpacker:install:typescript before – which is necessary to get the configuration files (like tsconfig.json). I think it would be better if we don‘t mix it up.

But: we may could add the ts/tsx extensions to the current Babel config in the Typescript install script. Then we could also remove the environment.loaders.delete('typescript') step, which you mentioned below.

Copy link
Member

Choose a reason for hiding this comment

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

I see, I think that's fine.

It wouldn't do anything for people who aren't using typescript but will probably fail if they are using typescript and don't have tsconfig.json? But perhaps not, since some of default options would on in the preset itself, right?

Copy link
Member

Choose a reason for hiding this comment

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

Could you please test by including extensions as default?

@@ -1,3 +1,4 @@
const { environment } = require('@rails/webpacker')
environment.loaders.delete('typescript')
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to do this?

@gopeter
Copy link
Contributor Author

gopeter commented Apr 16, 2020

Hi @gauravtiwari. I've added the ts/tsx extensions to the babel-loader now. Much cleaner and works fine! 😄

Users still have to run bundle exec rails webpacker:install:typescript to use it, since ts/tsx files are ignored unless they get added to the webpacker.yml config during the installation step.

@gauravtiwari gauravtiwari merged commit 7755023 into rails:master Apr 19, 2020
@gauravtiwari
Copy link
Member

Awesome, thanks for working on it 🍰

@gopeter
Copy link
Contributor Author

gopeter commented Apr 19, 2020

Great! Thanks for merging! 😄

@noelrappin
Copy link

Hey, does this have a recommended upgrade path for existing TypeScript users who upgrade to 5.1?

@gopeter
Copy link
Contributor Author

gopeter commented Apr 20, 2020

Not yet, I‘m going to make a new PR today with docs for it.

Maybe we can also add a rake task for it?

@gopeter
Copy link
Contributor Author

gopeter commented Apr 20, 2020

@noelrappin & @gauravtiwari I've created #2541. Sorry for not thinking about this before 😞

@noelrappin
Copy link

@gopeter please see #2558 and let me know if there is something wrong...

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.

None yet

3 participants