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
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions docs/testing.md
Expand Up @@ -10,8 +10,7 @@ Webpacker does not setup `Karma` by default, so you've to manually install it al
"test": "NODE_ENV=test karma start"
},
"dependencies": {
"typescript": "^2.5.2",
"ts-loader": "^2.3.7"
"typescript": "^2.5.2"
},
"devDependencies": {
"karma": "^1.7.1",
Expand Down
24 changes: 4 additions & 20 deletions docs/typescript.md
Expand Up @@ -21,35 +21,19 @@ bundle exec rails webpacker:install:typescript
```

2. Rename generated `hello_vue.js` to `hello_vue.ts`.
3. Add the webpack plug-n-play plugin to your yarn packages with `yarn add pnp-webpack-plugin`.
4. Change the generated `config/webpack/loaders/typescript.js` from
3. Change the generated `babel.config.js` from

```js
module.exports = {
test: /\.tsx?(\.erb)?$/,
use: [{
loader: 'ts-loader'
}]
}
["@babel/preset-typescript", { "allExtensions": true, "isTSX": true }]
```

to

```js
const PnpWebpackPlugin = require('pnp-webpack-plugin');

module.exports = {
test: /\.tsx?(\.erb)?$/,
use: [{
loader: 'ts-loader',
options: PnpWebpackPlugin.tsLoaderOptions({
appendTsSuffixTo: [/\.vue$/]
})
}]
}
["babel-preset-typescript-vue", { "allExtensions": true, "isTSX": true }]
```

and now you can use `<script lang="ts">` in your `.vue` component files. See [the pnp-webpack-plugin docs for the `ts-loader` integration](https://github.com/arcanis/pnp-webpack-plugin#ts-loader-integration) for more info.
and now you can use `<script lang="ts">` in your `.vue` component files. See [the babel-preset-typescript-vue docs](https://www.npmjs.com/package/babel-preset-typescript-vue) for more info.

## HTML templates with Typescript and Angular

Expand Down
1 change: 1 addition & 0 deletions lib/install/config/webpack/environment.js
@@ -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?


module.exports = environment
3 changes: 2 additions & 1 deletion lib/install/examples/react/tsconfig.json
Expand Up @@ -8,7 +8,8 @@
"moduleResolution": "node",
"sourceMap": true,
"target": "es5",
"jsx": "react"
"jsx": "react",
"noEmit": true
},
"exclude": [
"**/*.spec.ts",
Expand Down
3 changes: 2 additions & 1 deletion lib/install/examples/typescript/tsconfig.json
Expand Up @@ -11,7 +11,8 @@
"*": ["node_modules/*", "app/javascript/*"]
},
"sourceMap": true,
"target": "es5"
"target": "es5",
"noEmit": true
},
"exclude": [
"**/*.spec.ts",
Expand Down
11 changes: 0 additions & 11 deletions lib/install/loaders/typescript.js

This file was deleted.

22 changes: 11 additions & 11 deletions lib/install/typescript.rb
Expand Up @@ -15,17 +15,14 @@
end
end

say "Copying typescript loader to config/webpack/loaders"
copy_file "#{__dir__}/loaders/typescript.js", Rails.root.join("config/webpack/loaders/typescript.js").to_s
say "Don't remove TypeScript loader"
gsub_file Rails.root.join("config/webpack/environment.js").to_s,
/environment\.loaders\.delete\('typescript'\)\n/, ""

say "Adding typescript loader to config/webpack/environment.js"
insert_into_file Rails.root.join("config/webpack/environment.js").to_s,
"const typescript = require('./loaders/typescript')\n",
after: /require\(('|")@rails\/webpacker\1\);?\n/

insert_into_file Rails.root.join("config/webpack/environment.js").to_s,
"environment.loaders.prepend('typescript', typescript)\n",
before: "module.exports"
say "Adding TypeScript preset to babel.config.js"
insert_into_file Rails.root.join("babel.config.js").to_s,
",\n ['@babel/preset-typescript', { 'allExtensions': true, 'isTSX': true }]",
before: /\s*\].filter\(Boolean\),\n\s*plugins: \[/

say "Copying tsconfig.json to the Rails root directory for typescript"
copy_file "#{__dir__}/examples/#{example_source}/tsconfig.json", "tsconfig.json"
Expand All @@ -41,6 +38,9 @@
"#{Webpacker.config.source_entry_path}/hello_typescript.ts"

say "Installing all typescript dependencies"
run "yarn add typescript ts-loader #{additional_packages}"
run "yarn add typescript #{additional_packages}"

say "Installing all typescript dev-dependencies"
run "yarn add @babel/preset-typescript babel-preset-typescript-vue -D"

say "Webpacker now supports typescript 🎉", :green
5 changes: 2 additions & 3 deletions lib/webpacker/compiler.rb
Expand Up @@ -20,9 +20,8 @@ def compile
if stale?
run_webpack.tap do |success|
# We used to only record the digest on success
# However, the output file is still written on error, (at least with ts-loader), meaning that the
# digest should still be updated. If it's not, you can end up in a situation where a recompile doesn't
# take place when it should.
# However, the output file is still written on error, meaning that the digest should still be updated.
# If it's not, you can end up in a situation where a recompile doesn't take place when it should.
# See https://github.com/rails/webpacker/issues/2113
record_compilation_digest
end
Expand Down
4 changes: 2 additions & 2 deletions package/environments/__tests__/base.js
Expand Up @@ -40,8 +40,8 @@ describe('Environment', () => {
const defaultRules = Object.keys(rules)
const configRules = config.module.rules

expect(defaultRules.length).toEqual(7)
expect(configRules.length).toEqual(8)
expect(defaultRules.length).toEqual(8)
expect(configRules.length).toEqual(9)
})

test('should return default plugins', () => {
Expand Down
4 changes: 3 additions & 1 deletion package/rules/index.js
@@ -1,4 +1,5 @@
const babel = require('./babel')
const typescript = require('./typescript')
const file = require('./file')
const css = require('./css')
const sass = require('./sass')
Expand All @@ -16,5 +17,6 @@ module.exports = {
moduleCss,
moduleSass,
nodeModules,
babel
babel,
typescript
}
21 changes: 21 additions & 0 deletions package/rules/typescript.js
@@ -0,0 +1,21 @@
const { join, resolve } = require('path')
const { cache_path: cachePath, source_path: sourcePath, resolved_paths: resolvedPaths } = require('../config')
const { nodeEnv } = require('../env')

// 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?

include: [sourcePath, ...resolvedPaths].map((p) => resolve(p)),
exclude: /node_modules/,
use: [
{
loader: 'babel-loader',
options: {
cacheDirectory: join(cachePath, 'babel-loader-node-modules'),
cacheCompression: nodeEnv === 'production',
compact: nodeEnv === 'production'
}
}
]
}