-
-
Notifications
You must be signed in to change notification settings - Fork 372
Add support for typescript #11
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
Conversation
9a1fe84 to
ac2e208
Compare
|
Got most of it working 🎉 I'm surprised how easy that was. |
src/lib/ts-config.js
Outdated
| @@ -0,0 +1,10 @@ | |||
| export default { | |||
| ompilerOptions: { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a c here 😸
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, nice find! 🙈
src/lib/webpack-config.js
Outdated
| test: /\.tsx?$/, | ||
| use: [ | ||
| { | ||
| loader: resolve(__dirname, './npm-install-loader'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awwww yeah. So awesome.
|
Like with the less and sass installs, we should let people provide a |
|
That's a great idea, I like 👍 💯 |
|
We can do that bit after this PR if you want. |
src/lib/webpack-config.js
Outdated
| useBabel: true, | ||
| babelOptions: createBabelConfig(env), | ||
| useCache: true, | ||
| configFileName: resolve(__dirname, 'ts-config.js'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tsconfig.json is the standard config file name. Is there any particular reason for requiring a non-standard config file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that it is the standard name in the typescript ecosystem, same as with .babelrc for babel. Reason I went with this naming scheme is to be consistent with the other files in preact-cli. There isn't a .babelrc either, but babel-config.js instead. So I went with ts-config.js.
Plan is to use a user defined tsconfig.json if it is present at the projects root.
|
@developit I was just looking at how the |
|
Ah yeah I should have mentioned that one we didnt' get around to adding. It'd just be conditionally appending to the list of things to install in the new repo. edit: alright I just submitted a PR that adds those flags, should be easy to add the ts ones too: |
6643b61 to
e5a8c12
Compare
|
@jmfirth Seems like @developit I think it's ready to be merged now 🎉 |
49130ee to
71e3692
Compare
src/lib/webpack-config.js
Outdated
| options: { | ||
| sourceMap: true, | ||
| useBabel: true, | ||
| babelOptions: createBabelConfig(env), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder - if we enabled babel-loader for /.tsx?/ files, could we drop the useBabel and config here? TS files would get passed through awesome-typescript-loader (thanks to enforce:pre) and then later transformed from ES2017 to JS-of-yesteryear via the existing babel-loader rule.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory that should work, but somehow babel-loader is skipped if I don't explicitly add the babel config here. Perhaps @s-panferov or @TheLarkInn may help here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would require adding .tsx? to the babel-loader regex (the extension is unchanged).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that, but somehow module parsing fails to pass through babel:
./components/hello.tsx
Module parse failed: /Users/marvinhagemeister/dev/github/marvinhagemeister/preact-cli/lib/lib/npm-install-loader.js??ref--5-0!/Users/marvinhagemeister/dev/test/preact-cli-ts/boof/node_modules/awesome-typescript-
loader/dist/entry.js??ref--5-1!/Users/marvinhagemeister/dev/test/preact-cli-ts/boof/components/hello.tsx Unexpected token (2:11)
You may need an appropriate loader to handle this file type.
| export function Hello() {
| return <h1>Hello World</h1>;
| }
|
@ /Users/marvinhagemeister/dev/github/marvinhagemeister/preact-cli/~/babel-loader/lib?{"cacheDirectory":true,"plugins":["/Users/marvinhagemeister/dev/github/marvinhagemeister/preact-cli/~/babel-plugin-transform
-object-assign/lib/index.js","/Users/marvinhagemeister/dev/github/marvinhagemeister/preact-cli/~/babel-plugin-transform-decorators-legacy/lib/index.js","/Users/marvinhagemeister/dev/github/marvinhagemeister/prea
ct-cli/~/babel-plugin-transform-react-constant-elements/lib/index.js","/Users/marvinhagemeister/dev/github/marvinhagemeister/preact-cli/~/babel-plugin-transform-react-remove-prop-types/lib/index.js",["/Users/mar
vinhagemeister/dev/github/marvinhagemeister/preact-cli/~/babel-plugin-transform-react-jsx/lib/index.js",{"pragma":"h"}],["/Users/marvinhagemeister/dev/github/marvinhagemeister/preact-cli/~/babel-plugin-jsx-pragm
atic/jsx-pragmatic.js",{"module":"preact","export":"h","import":"h"}]],"presets":[["/Users/marvinhagemeister/dev/github/marvinhagemeister/preact-cli/~/babel-preset-env/lib/index.js",{"loose":true,"modules":false
,"uglify":true,"browsers":["> 1%","Last 2 versions","IE >= 9"],"exclude":["transform-regenerator","transform-es2015-typeof-symbol"]}],"/Users/marvinhagemeister/dev/github/marvinhagemeister/preact-cli/~/babel-pre
set-stage-0/lib/index.js"]}!./routes/home/index.js 11:0-47
@ ./routes/home/index.js
@ ./index.js
@ /Users/marvinhagemeister/dev/github/marvinhagemeister/preact-cli/lib/lib/entry.js
@ multi /Users/marvinhagemeister/dev/github/marvinhagemeister/preact-cli/lib/lib/entry webpack-dev-server/client?http://0.0.0.0:8080/ webpack/hot/dev-server?http://0.0.0.0:8080/There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could compile the jsx part directly via ts, but I'm not sure if babel jsx optimizations will work anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only one we have enabled is hoisting, which rarely gets triggered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's with useBabel turned off for awesome-ts-loader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not getting back any sooner, I was on vacation the past week. Yep, that's with useBabel turned off. I'm not sure why the pipelining fails. My initial assumption was that plugins in webpack enforced via pre are executed before everything else and that this would turn .ts into .js and then the babel loader would match. Somehow this is not the case here. I could try swapping awesome-typescript-loader with ts-loader. The latter seems to have made huge improvements in the last months.
|
@marvinhagemeister Any idea when this might be ready to go in? |
|
This is a great ticket, really looking for starting develop with preact-cli and TypeScript! Guys, my +100 internets for you! |
62da851 to
328bfce
Compare
328bfce to
7f029c2
Compare
|
@ethanroday @pqr I'm not sure when this will be ready to merge again. Just did a rebase against master, but it seems like in the time between opening this PR and now a few internals have changed. File changes are not picked up by webpack when changing a ts-file. Will have to dig deeper as to what's causing this. If anyone wants to chime in: Help is always greatly appreciated. I'm currently limited with time. |
| useBabel: true, | ||
| babelOptions: createBabelConfig(env), | ||
| useCache: true, | ||
| configFileName: tsconfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if tsconfig.json being one directory level up is what's affecting this? We moved the webpack modules into lib/webpack/* but tsconfig is still in lib/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a side-note, it's working fine for me with ts-loader via this preact.config.js hack (with tsconfig.json in the root of the project):
export default function (config, env, helpers) {
config.module.loaders.push({
enforce: "pre",
test: /\.tsx?$/,
loader: "ts-loader"
});
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wub How is your experience so far with ts-loader. I've only used them a few months back and awesome-typescript-loader was a lot faster back then.
@developit I'm honestly thinking of closing this PR as now the ability to extend the webpack config has been added. That way the core cli remains lean and a config like @wub posted above is as easy as it gets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@developit @marvinhagemeister should I add above to the preact.config.js recipes and abandon this PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to have some example how to setup preact-cli to work with TypeScript with css modules
I am struggling with this 💢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rkostrzewski yep.
@porfirioribeiro CSS-Modules should work out of the box. The only difference is that you'll have to revert to require statements:
// styles appears as type any, because ts has no idea of `.css` files
const styles = require("foo.css");There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marvinhagemeister awesome-typescript-loader is still much faster, although you can reach similar speeds with ts-loader with the fork checker plugin. I'm going to switch back to ATL, because all this stuff (including babel support) is "first-class."
| ] : []), | ||
|
|
||
| // install typescript setup if --typescript | ||
| ...(argv.typescript ? [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since noEmitHelpers is set to true, can we also add tslib as dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since babel is applied right after the ts step and brings its own set helpers, I don't think adding the weight of another helper library is beneficial.
| "noEmitHelpers": true, | ||
| "module": "es2015", | ||
| "moduleResolution": "node", | ||
| "target": "es2017", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be set to 2015?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, the ts should compile the latest js version possible because babel is applied right after that anyways.
|
Not to resurrect this - but would anyone be up for releasing a |
|
@developit I've done the first cut here: https://github.com/wub/preact-cli-plugin-typescript Would appreciate any feedback/pull requests from the community. |
|
It would nice to have the CLI generate a sample app that uses .ts/.tsx files. That way developers that want to use TS have an application that runs out of the box without having to change file extensions and fix all the compilation errors; update css imports to use require, and missing type annotations, etc. |
|
With #329, we're moving to remote/foreign templates. This means that you can install any PreactCLI-compatible template to use what you like & still get all the great benefits of the CLI commands & configs. That said, I'm sure someone (likely from this PR) will volunteer to maintain a TS CLI starter. 👍 |
|
Nice, that looks perfect |
Currently a work an progress. Everything is compiling so far, but I get an exception at runtime.
Fixes #10
Tasks:
tsandtsxfilestsconfigwith stricter rules--typescriptsupport css modules viaThis is a general TS issue. For nowimportsyntaxrequire()calls workinstall typings for preact by defaultPreact already includes typings 👍