Add support to enableTypescriptLoader #50

davidmpaz · 2017-06-21T17:19:47Z

Hi,

please would you consider adding this to main stream?

this add public API to enable TypeScript loader (ts-loader). It follows much of what is described in #9

Cuurrently only add vanilla loader as described in here playing side by side with babel.

Future work would be to include also support to happypack and fork-ts-checker-webpack-plugin

Thanks in advance

weaverryan · 2017-06-21T17:33:35Z

Hey @davidmpaz! I would be very happy to accept this :). Can you rebase - I recently merged some PR's?

Future work would be to include also support to happypack and fork-ts-checker-webpack-plugin

Indeed - happypack is totally unrelated to TypeScript, correct? And fork-ts-checker-webpack-plugin simply adds extra type-checking (and so therefore, build errors/warnings), correct? I just want to minimize BC breaks - so I want to make sure "future things" won't be actually difficult to add later :).

weaverryan

I think this is already very close!

weaverryan · 2017-06-21T17:34:48Z

index.js

+     * @return {exports}
+     */
+    enableTypeScriptLoader(options = {}) {
+        webpackConfig.enableTypeScriptLoader(options);


I think options should be a callback - like we do with configureBabel(), so that we don't need to worry about merging about default options and their options.

Good point and will update that.

weaverryan · 2017-06-21T17:36:33Z

lib/WebpackConfig.js

+        for (const optionKey of Object.keys(options)) {
+            if (!(optionKey in this.typeScriptOptions)) {
+                throw new Error(`Invalid option "${optionKey}" passed to enableTypeScriptLoader(). Valid keys are ${Object.keys(this.typeScriptOptions).join(', ')}`);
+            }


I usually like adding nice validation like this, but I'm afraid maintaining this will be a pain (we need to add a new option whenever they add a new option). The typescript-loader should ideally validate these keys themselves.

More about what we talked already. Is a good point and I will update the PR to take this into account.

weaverryan · 2017-06-21T17:37:26Z

package.json

    "resolve-url-loader": "^2.0.2",
    "style-loader": "^0.13.2",
    "webpack": "^2.2.0",
    "webpack-chunk-hash": "^0.4.0",
-    "webpack-dev-server": "^2.4.5",
+    "webpack-dev-server": "^2.5.0",


Let's not bump any versions in this PR - it seems unrelated

That was me being neglectful, sorry about that ^ ^ it was not intended.

weaverryan · 2017-06-21T17:37:51Z

test/functional.js

+                webpackAssert.assertOutputFileContains(
+                    'main.js',
+                    'document.getElementById(\'wrapper\').innerHTML = "<h1> Hello World!</h1>";'
+                );


great way to test it's working - I like that!

Btw, the test page is currently blank, but for the vue loader, I'm adding a <div id="app"></div> to it in setup.js where testing.html is created for the functional tests. You could do the same and then assert that the app element actually has text in it (I think that should work - I originally thought that's what you were doing)

The original intend was to check that we got something expected in the compiled module. It was just about compilation going through and check that the file was imported.

Did you mean that inside that functional test one can make reference to document.getElementById('app') ? It can be done of course. But how is this checked later on? and how will this be usefull for the tests ?

Yes! Zombie executes the JavaScript, so we can assert that the final elements exist on the page correctly (i.e. after the TypeScript code has executed). I've just done this for the vuejs loader - here's a quick gist: https://gist.github.com/weaverryan/64cae3f603529641dcad781eeb0cdcd9

Thanks! 👍 I could reproduce it for TS. Just one thing. I needed to add the div#app to html generated for functional test. Is this correct? I was not sure if the html was added by some of the scripts included.

weaverryan · 2017-06-21T17:39:42Z

fixtures/js/render.ts

@@ -0,0 +1,5 @@
+function render() {


What about renaming this to render.tsx - it would slightly enhance the test, since it would take advantage of the resolve.extensions change and make sure the loader applies to both extensions

Very good idea! 👍

weaverryan · 2017-06-21T17:42:34Z

lib/WebpackConfig.js

+            compilerOptions: {},
+            entryFileIsJs: false,
+            appendTsSuffixTo: []
+        };


When we remove the validation below, we should be able to remove these, correct (since these are just the default values)?

These are the same default values taken from ts-loader I add them just to follow the same scheme of implementation Encore had, but if that validation will be removed we can remove also those defaults.

Also if configuration by callback is added then makes no sense since we are delegating all of that to users. Make sense to make other validations for sanity check but not these currently.

davidmpaz · 2017-06-22T08:20:00Z

Hey @weaverryan I have updated PR. To answer some of your questions:

happypack is totally unrelated to TypeScript, correct?

Yes. These are the loaders currently known to be supported: https://github.com/amireh/happypack/wiki/Loader-Compatibility-List. This plugin IMHO is critical when having slow build, for now it can be added seperated to Encore but will be nice to have this one also included. I havent check yet other solutions to reduce build time.

And fork-ts-checker-webpack-plugin simply adds extra type-checking (and so therefore, build errors/warnings), correct?

This one makes the types checking in separate thread, meaning that TypeScript compiler doesn't need to do it, making compilation time faster by setting transpileOnly = true to loader. Actually I did not add this one as "default" setup in loaders because eslint was complaining about the module not being published, not sure what that was.

But actually this could be included as bonus on Encore alongside with ts-loader. Encore could handle transparently the configuration in case users decide to enable the type checking in separate thread to speed up compilation/build time. In my case I reduced several (~2-3k) ms.

weaverryan

A few more notes - but this is very close!

weaverryan · 2017-06-23T02:00:15Z

.gitignore

@@ -1,3 +1,4 @@
 node_modules/
 npm-debug.log*
 /test_tmp
+.idea


You can remove this - it should be set in people's global .gitignore

Good point.

weaverryan · 2017-06-23T02:02:29Z

index.js

+     * @return {exports}
+     */
+    configureTypeScript(callback) {
+        webpackConfig.configureTypeScript(callback);


Sorry to go back and forth, but I was thinking more about this - the callback is more difficult to understand and quite a bit harder to use (especially if you want to add a bunch of options at once - you can't just copy some {foo: bar, baz: true} code from somewhere and paste it in). I now think we should have this as a normal options object (like you had before). If we eventually add more default options and merging gets weird, we could deprecate the options object and force a callback.

Don't worry about that, the idea is to get to the best solution possible.

Regarding why I did accept your callback suggestion before. I indeed thought the same as you, on seconds thoughts, after looking the code again, Is it better to keep it like options!?

My conclusion was: no. Why? Because I think it is more explicit to have a callback which accept parameters to configure instead having to know the structure of an object configuration that configure several aspects of application. for example.

This is how i thought on how could be integrated other stuff better in future:

fork-ts-checker-webpack-plugin: When tried to add this in ts-loader configuration did not feel natural since it is another plugin that can be configured by its own. Still is only useful if you use ts-loader and with the option: transpileOnly: true. In future this could be passed as another config object to callback.

I also thought on the happypack plugin. This one is more general and can be used with many loaders, but still, having the call back would help at the time of configuration because you will have to configure a bit different the loaders when using it.

Because of this I did not question the callback idea. And that's why I think it would be better like this. Maybe thinking about adding parameters to a configuration callback could not be an optimal solution either, in which case I am open to suggestions :)

What do you think about it?

+1 Let's keep it then :) (and we can always add support for the simple object later... if it proves that it's really a problem [unlikely]). I'll update #49 to use this same method.

Oh, and one more thing: let's rename this to enableTypeScriptLoader() to be consistent with the other loaders (configureBabel() is the outlier - because Babel is always enabled, so we're just configuring it).

weaverryan · 2017-06-23T02:03:17Z

lib/config-generator.js

+        let extensions = ['.js', '.jsx'];
+        if (this.webpackConfig.useTypeScriptLoader) {
+            extensions = extensions.concat(['.json', '.ts', '.tsx']);
+        }


See my 2nd bullet point here: #49 (comment). Perhaps we should add .ts and .tsx to extensions always.

I see, that's very good to have. Could you please point me where this validation occur? I just can not find it now :(

weaverryan · 2017-06-23T02:06:04Z

lib/loaders/typescript.js

+            happyPackMode: false,
+            logInfoToStdOut: false,
+            logLevel: 'info',
+            silent: true,


It looks like these are already all of the default values (except for silent), so can we just remove them instead? And why silent: true?

You are right! I thought on being more verbose there to have it documented in code but it is right.

And why silent: true?

Avoiding to be verbose with CLI output. As show here it doesn't affect processing of files, only output. For example without silent it is always printed messages about which tsconfig.json is used. I think it was too much and was making also the output of test not nice looking since it was printing out of context.

Basically it is more a cosmetic change.

weaverryan · 2017-06-23T02:06:49Z

lib/loaders/typescript.js

+
+        // use ts alongside with babel
+        // @see https://github.com/TypeStrong/ts-loader/blob/master/README.md#babel
+        let loaders = babelLoader.getLoaders(webpackConfig);


love the note + link 👍

weaverryan · 2017-06-23T02:07:17Z

package-lock.json

@@ -0,0 +1,5187 @@
+{


remove this package-lock.json file - we're using yarn internally in the library

Better! I don't know if it is me, but I have faced problems with npm. The package management is not consistent. Some times I don't get all my dependencies installed.

I am wondering now whether this is the reason why you use yarn? :)

Well, yarn is a better solution than npm 3. And npm 5 was not yet released when we started this project

weaverryan · 2017-06-23T02:09:11Z

test/functional.js

+            config.addEntry('main', ['./js/CoolReactComponent.jsx', './js/render2.tsx']);
+            config.enableReactPreset();
+            config.configureTypeScript(function(tsConfig) {
+                tsConfig.compilerOptions = { 'jsx': 'preserve' };


Can you tell me about this option? And why would react and typescript cause problems when enabled together? Is there some known issue that we want to make sure isn't a problem?

It is a TypeScript compilation issue. Remove it and you get compilation errors about not having configure this option. This only happen when compiling .tsx files. I did not add a check/validation for that when configuring loader because:

Not sure where to add it.

Difficult to know before hand which kind of files will be parsed ? Not sure here though.

The compilation error is pretty clear. Developers should configure the option, that's it.

Take a look to this example configuration I just tooked from there. I am not an expert so most of test/functional/usage I have took them from ts-loader project itself

Interesting... I just tried your code locally - and the test passed without the .jsx: 'preserve' option. If you comment out setting that option, does this test fail for you? Perhaps we would actually need to add some JSX into the .ts files (e.g. https://github.com/TypeStrong/ts-loader/blob/master/examples/react-babel-karma-gulp-happypack/src/main.tsx). And if the error is really clear and we're not doing anything special to handle it, does the test add anything? Or, if the babel preset has already been activated, should we set this option for them (or perhaps too magic)? If we're not sure, I'd prefer to do nothing and let the user configure this themselves.

davidmpaz · 2017-06-23T07:10:14Z

Hey @weaverryan I have added some comments, let me know your thoughts. I need to leave the updates for the weekend probably, if not, on Monday I make a new round.

thanks for the feedback and looking forward.

cheers

* Add public api for enable loader. * Add default options taken from ts-loader docs. * Add loader to generated configuration. * Update packages.json dependencies * Add loader to loader features to check later.

Fix typos and copy/paste errors.

Add extensions to resolve from type script.

Remove options and add callback for configuraiton * Add new loader util for typescript. * Refactor public API signature.

* Update fixtures to insert content inside #app div * Add assertion for requestTestPage and files generated.

This reverts commit a02d8ab.

weaverryan

Thanks! Another round! Another step closer :)

weaverryan · 2017-06-25T13:54:47Z

index.js

+     * @return {exports}
+     */
+    configureTypeScript(callback) {
+        webpackConfig.configureTypeScript(callback);


+1 Let's keep it then :) (and we can always add support for the simple object later... if it proves that it's really a problem [unlikely]). I'll update #49 to use this same method.

weaverryan · 2017-06-25T13:55:18Z

lib/config-generator.js

+        let extensions = ['.js', '.jsx', '.ts', '.tsx'];
+        if (this.webpackConfig.useTypeScriptLoader) {
+            extensions.push('.json');
+        }


What's special about TypeScript and the .json extension?

Odd!... I recall having issues with loader not resolving the tsconfig.json file for TypeScript compilation if not added the json extension. But now I see on my project everything is fine without it and also the tests are fine. So I'll remove this.

weaverryan · 2017-06-25T14:00:53Z

lib/friendly-errors/transformers/missing-loader.js

+        case 'tsx':
+        case 'ts':
+            error.loaderName = 'typescript';
+            break;


Good catch on this! As it looks like you discovered, this is what gives us the good error when you try to load a .tsx file and you haven't enabled its loader :)

weaverryan · 2017-06-25T14:02:01Z

lib/test/setup.js

@@ -140,6 +140,7 @@ function requestTestPage(webRootDir, scriptSrcs, callback) {
 <head>
 </head>
 <body>
+    <div id="app"></div>


+1 this is the correct way to do that (I did it in #49 too - we'll just merge them together)

weaverryan · 2017-06-25T14:03:41Z

index.js

+     * @return {exports}
+     */
+    configureTypeScript(callback) {
+        webpackConfig.configureTypeScript(callback);


Oh, and one more thing: let's rename this to enableTypeScriptLoader() to be consistent with the other loaders (configureBabel() is the outlier - because Babel is always enabled, so we're just configuring it).

weaverryan · 2017-06-25T14:04:03Z

test/WebpackConfig.js

+            const config = createConfig();
+            const testCallback = () => {};
+            config.configureTypeScript(testCallback);
+            expect(config.tsConfigurationCallback).to.equal(testCallback);


We should have 1 test that passes no callback, to make sure that works

weaverryan · 2017-06-25T14:16:42Z

test/functional.js

+            config.addEntry('main', ['./js/CoolReactComponent.jsx', './js/render2.tsx']);
+            config.enableReactPreset();
+            config.configureTypeScript(function(tsConfig) {
+                tsConfig.compilerOptions = { 'jsx': 'preserve' };


Interesting... I just tried your code locally - and the test passed without the .jsx: 'preserve' option. If you comment out setting that option, does this test fail for you? Perhaps we would actually need to add some JSX into the .ts files (e.g. https://github.com/TypeStrong/ts-loader/blob/master/examples/react-babel-karma-gulp-happypack/src/main.tsx). And if the error is really clear and we're not doing anything special to handle it, does the test add anything? Or, if the babel preset has already been activated, should we set this option for them (or perhaps too magic)? If we're not sure, I'd prefer to do nothing and let the user configure this themselves.

weaverryan · 2017-06-25T14:17:42Z

test/functional.js

+                        // assert that the ts module rendered
+                        browser.assert.text('#app h1', 'Welcome to Your TypeScript App');
+                        // make sure the styles are not inline
+                        browser.assert.elements('style', 0);


I think we can remove this for this test - I had it for vue, because vue actually allows you to put styles inline in your .vue files, so it was relevant to check that those had been processed correctly.

Interesting... I just tried your code locally - and the test passed without the .jsx: 'preserve' option.

For me I was getting an error, clear how to solve, but error at last. Currently is not like that. Also odd! Maybe I was just tired :). I will remove the option since it is not throwing any error and on build time users will be able to notice what to do clearly.

I think we can remove this for this test

done

weaverryan · 2017-06-25T14:29:12Z

One more thing - can you run yarn install - it should create an updated yarn.lock file that needs to be committed.

* Remove .json from resolved extensions. * Remove not needed configuration options in tests. * Remove assertion on tests for styles.

weaverryan

I added one comment about a test I think we should remove. But, this is 👍 from me!

weaverryan · 2017-06-26T14:12:22Z

test/functional.js

+            });
+        });
+
+        it('When enabled, react JSX and TypeScript work nice together!', (done) => {


I think we should remove this test entirely. Afaik, I think you will get an error if you add some JSX into a .tsx file (that clear error you talked about). But since we're not doing anything in Encore to work around this or help in any way, this test isn't adding any value.

Sounds good, could you please take care of removing when merging ? From my side will take bit longer now.

thanks in advance 👍

weaverryan · 2017-06-29T19:40:38Z

Thanks for your awesome and hard work on this David! I also created a docs issue to document this - would you be interested in going at that too? :)

…yan) This PR was merged into the master branch. Discussion ---------- Add support to enableTypescriptLoader Hi, please would you consider adding this to main stream? this add public API to enable TypeScript loader ([ts-loader](https://github.com/TypeStrong/ts-loader)). It follows much of what is described in #9 Cuurrently only add vanilla loader as described in [here](https://github.com/TypeStrong/ts-loader/blob/master/examples/vanilla/webpack.config.js) playing side by side with babel. Future work would be to include also support to `happypack` and `fork-ts-checker-webpack-plugin` Thanks in advance Commits ------- 6d5dee8 Fixing bad merge 35d9e6f Merge branch 'master' into typescript 82e17e3 Remove unnecessary functional test 4d5319d Update yarn.lock with typescript deps 144acd2 Cleaning up unnecessary code. f6bc6e7 Rename to enableTypeScriptLoader facc132 Remove package-lock.json 25b9f18 Remove duplicated default configuration options 23619b2 Add test for error handling on missing ts loader d887a79 Add typescript to missing loader error handling 61370ac Add .ts & .tsx to resolved extension 6ec2b52 Revert "Add .idea directory to gitignore rules" 356aaa2 Make tests check for content in browser context 822ceff Improve tests for compiling .tsx files ae1bea3 Update packages-lock.json file a02d8ab Add .idea directory to gitignore rules 32c2853 Revert accidental version change 846d142 Update tests to take into account refactoring b43f5c9 Refactor to favor configure by callback 1352d1c Add missing extensions to config.resolve 7e8c118 Fix copy'n paste errors 58b7ef7 Add unit and functional test for TypeScript loader fc26e1b Add support to enable TypeScript loader

davidmpaz · 2017-06-30T06:36:58Z

It was my pleasure :) ... I am interested to having it available built in as soon as possible.

Regarding docs, sure. Please reference the issue and if possible/exist point me to some guidelines on how do you tackle docs in the project.

UPDATE: Please could you add the missing comma here I think with the merge went out :)

This PR was squashed before being merged into the master branch (closes #64). Discussion ---------- Fix missing comma and lint index This fix #62 and adds index.js to lint task to avoid such things in future. Missing comma is related to #50. I believe the merge remove it by accident. Commits ------- e44bd38 Add index.js to linter 9675c1b Add missing comma

weaverryan · 2017-07-02T13:59:05Z

Yea, sorry about that comma - bad merge on my part indeed! The docs issue is here: symfony/symfony-docs#8100. And here's the PR I made (as an example) for vue: symfony/symfony-docs#8101

weaverryan requested changes Jun 21, 2017

View reviewed changes

weaverryan requested changes Jun 23, 2017

View reviewed changes

David Paz and others added 17 commits June 24, 2017 09:07

Add support to enable TypeScript loader

fc26e1b

* Add public api for enable loader. * Add default options taken from ts-loader docs. * Add loader to generated configuration. * Update packages.json dependencies * Add loader to loader features to check later.

Add unit and functional test for TypeScript loader

58b7ef7

Fix copy'n paste errors

7e8c118

Fix typos and copy/paste errors.

Add missing extensions to config.resolve

1352d1c

Add extensions to resolve from type script.

Refactor to favor configure by callback

b43f5c9

Remove options and add callback for configuraiton * Add new loader util for typescript. * Refactor public API signature.

Update tests to take into account refactoring

846d142

Revert accidental version change

32c2853

Add .idea directory to gitignore rules

a02d8ab

Update packages-lock.json file

ae1bea3

Improve tests for compiling .tsx files

822ceff

Make tests check for content in browser context

356aaa2

* Update fixtures to insert content inside #app div * Add assertion for requestTestPage and files generated.

Revert "Add .idea directory to gitignore rules"

6ec2b52

This reverts commit a02d8ab.

Add .ts & .tsx to resolved extension

61370ac

Add typescript to missing loader error handling

d887a79

Add test for error handling on missing ts loader

23619b2

Remove duplicated default configuration options

25b9f18

Remove package-lock.json

facc132

weaverryan requested changes Jun 25, 2017

View reviewed changes

This was referenced Jun 25, 2017

Vue.js Loader #49

Merged

Unable to add custom options for sass compilation #14

Closed

ghost mentioned this pull request Jun 26, 2017

Add loader for Typescript #9

Closed

David Paz added 3 commits June 26, 2017 13:10

Rename to enableTypeScriptLoader

f6bc6e7

Cleaning up unnecessary code.

144acd2

* Remove .json from resolved extensions. * Remove not needed configuration options in tests. * Remove assertion on tests for styles.

Update yarn.lock with typescript deps

4d5319d

weaverryan approved these changes Jun 26, 2017

View reviewed changes

weaverryan mentioned this pull request Jun 26, 2017

quilljs with webpack-encore. #58

Closed

Remove unnecessary functional test

82e17e3

weaverryan mentioned this pull request Jun 29, 2017

[Encore] Document typescript-loader symfony/symfony-docs#8100

Closed

weaverryan and others added 2 commits June 29, 2017 14:08

Merge branch 'master' into typescript

35d9e6f

Fixing bad merge

6d5dee8

weaverryan merged commit 6d5dee8 into symfony:master Jun 29, 2017

davidmpaz mentioned this pull request Jun 30, 2017

Fix missing comma and lint index #64

Merged

davidmpaz deleted the typescript branch July 23, 2017 15:35

weaverryan mentioned this pull request May 9, 2020

[WIP] Failing tests if there are any unsilenced logs #761

Open

1 task

Add support to enableTypescriptLoader #50

Add support to enableTypescriptLoader #50

Conversation

davidmpaz commented Jun 21, 2017

weaverryan commented Jun 21, 2017

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmpaz Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmpaz commented Jun 22, 2017 • edited Loading

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmpaz commented Jun 23, 2017

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Jun 25, 2017

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Jun 29, 2017

davidmpaz commented Jun 30, 2017 • edited Loading

weaverryan commented Jul 2, 2017

davidmpaz Jun 22, 2017 •

edited

Loading

weaverryan Jun 21, 2017 •

edited

Loading

davidmpaz commented Jun 22, 2017 •

edited

Loading

stof Jun 23, 2017 •

edited

Loading

davidmpaz commented Jun 30, 2017 •

edited

Loading