Forked typescript type checking #101

davidmpaz · 2017-07-19T13:20:38Z

This PR fixes: #63 it should be possible to merge also with #99 directly.

Open points:

Regarding point .1 in #2

If this plugin is added. we need to check dependency is added using loader-features.js. This module name is getting name from loaders, we are not adding a loader here but a plugin. Maybe some more general module is needed for checking features?

After implementing this I realize that as soon as you require a module which doesn't exist node throws an error, we don't even go through error catch from encore. So possible solutions I see for this pending point are:

We don't refactor loader-features.js file and we still will get a loud error when trying to use some plugin not installed. From error is clear whats need to be done.
We refactor loader-features.js to be named more general and we include plugins dependencies also there as intended initially. But this way we need to check for missing packages earlier in WebpackConfig.js like I am doing now in this PR.

I like either solutions the same, I would like to get feedback on what do you guys think about it?

thanks in advance.

weaverryan · 2017-07-21T01:46:50Z

I know what I'd like: option 2:

We refactor loader-features.js to be named more general and we include plugins dependencies also there as intended initially. But this way we need to check for missing packages earlier in WebpackConfig.js like I am doing now in this PR.

Even if the normal error from Node is pretty good, I'd rather be able to make the error specific to Encore - e.g. "You run yarn XYZ in order to use enableForkedTypeScriptTypesChecking".

I'll leave a comment that might help with the issue of where you place the "check" for the necessary packages.

Cheers!

weaverryan · 2017-07-21T01:49:30Z

lib/loaders/typescript.js

@@ -11,6 +11,7 @@

 const loaderFeatures = require('../loader-features');
 const babelLoader = require('./babel');
+const forkedTypesPluginUtil = require('../plugins/forked-ts-types');


When you require this file, it requires fork-ts-checker-webpack-plugin. And THAT causes the big Node error in case fork-ts-checker-webpack-plugin isn't installed. What you can do is move this require statement down - it doesn't need to be on top:

if (webpackConfig.useForkedTypeScriptTypeChecking) {
loaderFeatures.ensureLoaderPackagesExist('forkedtypecheck');

const forkedTypesPluginUtil = require('../plugins/forked-ts-types');

}

require is most commonly done on top, but it's not a requirement.

That was my first solution. I did not commit it because I was hidding a dependency in the execution path of the library. I guess that since we use it to report back an error it should be fine.

I'll take care of it. thanks!!

davidmpaz · 2017-07-21T06:46:24Z

Btw @weaverryan ... any idea how can I deal with that timeout error I receive from functional.js tests on appveyor ? I am considering now whether remove that test or not :(

davidmpaz · 2017-07-21T14:22:56Z

I believe that this timeout error on Appveyor and now Travis:

1) Functional tests using webpack Basic scenarios. TypeScript is compiled and type checking is done in separate process!:
     Error: Timeout of 8000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

is related to the cached node_modules directory. I had same errors locally in non related tests (like now in travis) when changing branches which uses differents modules, after an npm install error was gone.

I will ignore them, after merging everything should be gone.

weaverryan · 2017-07-21T21:19:19Z

@davidmpaz Now that I look at this (nice work btw), is there any disadvantage to just always adding the forked type checking, if fork-ts-checker-webpack-plugin is enabled? I'd like Encore to try to automatically apply as many sane optimizations as it can. So, when you apply ts loader (but don't have fork-ts-checker-webpack-plugin installed), we could add a "recommendation" console.log() that you'll have better performance if you install it (this would be the first time we have one of these "recommendations" comments - but it's an idea I'd like to do more to help educate the user).

davidmpaz · 2017-07-22T07:01:33Z

thanks. Regarding disadvantages I can see in their docs:

It's very important to be aware that this plugin uses typescript's, not webpack's modules resolution. It means that you have to setup tsconfig.json correctly. For example if you set files: ['./src/someFile.ts'] in tsconfig.json, this plugin will check only someFile.ts for semantic errors. It's because of performance.

And i did noticed that the plugin expect a tsconfig.json file from here if not, error is thrown. As far I remember for the ts-loader is recommended but optional, not a requirement.

Besides that, nothing more come to my mind.

weaverryan

@davidmpaz Ah, very good! Then you've made the right decision to not enable it by default. I hesitate with these decisions (to do more automatic than we do now). But instead of that, let's educate the users whenever we can recommend that they opt into something else.

Cheers!

weaverryan · 2017-07-25T00:52:15Z

index.js

+     *
+     * This is a build optimization API to reduce build times.
+     *
+     *


extra line here

What line is surplussing ? :)

We usually only have one empty (well *) line between the description and the start of the arguments. So, delete line 384 :).

weaverryan · 2017-07-25T00:54:23Z

index.js

+     * @param {Object} forkedTypeScriptTypesCheckOptions
+     * @return {exports}
+     */
+    enableForkedTypeScriptTypesChecking(forkedTypeScriptTypesCheckOptions = {}) {


We've made these options arguments callbacks everywhere else. Though it feels unnecessary here, I think we should do the same

weaverryan · 2017-07-25T00:55:31Z

lib/WebpackConfig.js

+
+        if (!this.useTypeScriptLoader) {
+            throw new Error('TypeScript loader must be enabled. Call `enableTypeScriptLoader()` first.');
+        }


I can't remember if I made this comment before or not, but checks like this need to be moved to be during the config generation process. It should be fine to call enableTypeScriptLoader() and enableForkedTypeScriptTypesChecking() in whatever order.

weaverryan · 2017-07-25T00:55:48Z

lib/loader-features.js

+    forkedtypecheck: {
+        method: 'enableForkedTypeScriptTypesChecking()',
+        packages: ['typescript', 'ts-loader', 'fork-ts-checker-webpack-plugin'],
+        description: 'check TypeScript types in separate process'


in a separate...

If you refer to make a feature checker for plugins I have some work related to that. I just want to start including something in main stream before make another PR for the loader features itself. For now I used like it is.

That would be cool - but I meant something simpler here :). Just a language tweak in the description: change in separate process to in a separate process - minor English thing!

weaverryan · 2017-07-25T00:57:29Z

lib/loaders/typescript.js

+            // add forked ts types plugin to the stack
+            const forkedTypesPluginUtil = require('../plugins/forked-ts-types');
+            forkedTypesPluginUtil(webpackConfig);
+        }


What about logging a recommendation here to enable this feature for faster type-checking? We now have a logger.js module that can be used for this (though I might add a new method on that - recommendation() - instead of using debug().

When we get that recommendation() method we can add it yes.

It's in #116 - we'll probably merge this first. If so, I'll add the warning after #116

weaverryan · 2017-07-25T00:58:10Z

package.json

@@ -34,6 +34,7 @@
    "css-loader": "^0.26.2",
    "extract-text-webpack-plugin": "^3.0.0",
    "file-loader": "^0.10.1",
+    "fork-ts-checker-webpack-plugin": "^0.2.7",


should be in devDependencies

Will get a linting error then. I even reported that in TypeStrong/fork-ts-checker-webpack-plugin#34 but it seems to be right like that.

Honestly I can not argue much about it since I still don't get right eslint rules :(

Ah, this makes sense. Congrats! You were the first to hit this issue :). Basically, it's quite often for Encore to have optional features. Think about sass-loader - we don't have that in our dependencies because we force the user to add this manually. We add it in devDependencies just so we can use it in our tests.

But fork-ts-checker-webpack-plugin is different, because (unlike all the optional loaders), we explicitly require this module (for loaders, you just mention the loader name and webpack requires it later - so the linter doesn't pick up on this).

tl;dr; The linter is correct in a sense: Encore is using this package in its actual code, not just in its tests. But it's wrong because we're manually checking to make sure this package exists before using it.

So, add // eslint-disable-line after the require line in forked-ts-types.js to silence this lint problem. THEN put it in devDependencies :)

weaverryan · 2017-07-25T00:58:45Z

test/WebpackConfig.js

+            }).to.throw(
+                'TypeScript loader must be enabled. Call `enableTypeScriptLoader()` first.'
+            );
+        });


Good test, but should live somewhere else after the corresponding code moves.

weaverryan · 2017-07-25T01:00:02Z

test/functional.js

+                // TODO find a way to confirm forked is used. Maybe terminal output?
+                // Output:
+                // Starting type checking service...
+                // Using 1 worker with 2048MB memory limit


hmm. Well, we have other tests that confirm that the config is setup correctly. Is it enough to know just that this test didn't blow up? Is there any reasonable way it could blow up? I'm probably ok without adding anything additional.

I don't know actually. That's why I did add the comment. Will check anyways.

Let's just remove these comments for now and leave the test as-is.

Lyrkan

Just some uses of let that can be replaced by const.

Lyrkan · 2017-07-28T19:41:36Z

lib/plugins/forked-ts-types.js

+
+'use strict';
+
+let ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin');


const instead of let

Lyrkan · 2017-07-28T19:42:10Z

test/plugins/forked-ts-types.js

+const WebpackConfig = require('../../lib/WebpackConfig');
+const RuntimeConfig = require('../../lib/config/RuntimeConfig');
+const tsLoader = require('../../lib/loaders/typescript');
+let ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin');


const instead of let

Lyrkan · 2017-07-28T19:44:19Z

lib/plugins/forked-ts-types.js

+ * @return {void}
+ */
+module.exports = function(webpackConfig) {
+    let config = Object.assign({}, webpackConfig.forkedTypeScriptTypesCheckOptions);


const instead of let

Add plugin at the time typescript loader is added.

As soon as we require a module that doesn't exist we get node error: "Cannot find module 'module-name'".

Add dependency and update yarn.lock

This is moved to typescript loader.

equire plugin module after packages check is done to avoid node error and print prettyfied error from encore.

Avoid timeout errors in Appveyor. Somehow x86 architecture is making mocha to timeout.

Call done callback before time expire.

Make module export function to meet similar signatures as for PR #99

Ignore eslint-disable-line rules to satisfay linter. See discussion: #101 (comment)

Test throwing Error on wrong configuration

weaverryan · 2017-08-05T12:11:15Z

Thank you David! If I have time this morning, I'll try to add the "recommendation" log to "advertise" this when you use ts-loader

…ly (weaverryan) This PR was merged into the master branch. Discussion ---------- Adding tests for the logger and cleaning up a bit internally ... also added the `recommendation` method, which we will use in #101. Commits ------- 7ecfece Adding tests for the logger and cleaning up a bit internally

weaverryan reviewed Jul 21, 2017

View reviewed changes

weaverryan requested changes Jul 25, 2017

View reviewed changes

weaverryan mentioned this pull request Jul 27, 2017

Adding tests for the logger and cleaning up a bit internally #116

Merged

Lyrkan requested changes Jul 28, 2017

View reviewed changes

davidmpaz added 21 commits July 30, 2017 11:07

Add type script type checking plugin public API

56f2099

Implement type checking plugin in encore configuration

da5f026

Add features needed by type checking plugin

8ceed5c

Add plugin implementation integrating checker in encore

2b9e4c5

Add forked type checker from typescript loader

fd37f05

Add plugin at the time typescript loader is added.

Add tests for new ts type checker plugin

d0b36cd

Call ensureLoaderPackagesExist ealier to avoid node error

7d391b0

As soon as we require a module that doesn't exist we get node error: "Cannot find module 'module-name'".

Add fork-ts-checker-webpack-plugin to packages.json

9e5dff4

Add dependency and update yarn.lock

Add return statement to allow chain calls

73ee341

Remmove browser request to reduce execution time

d74cfa9

Remove dependency check from WebpackConfig.js

07c740a

This is moved to typescript loader.

Move 'require' down after dependency check in loader

4cbc25c

equire plugin module after packages check is done to avoid node error and print prettyfied error from encore.

Increase timeout for forked functional test

0c6c850

Avoid timeout errors in Appveyor. Somehow x86 architecture is making mocha to timeout.

Make timeout match default value and call done()

666935d

Call done callback before time expire.

Add test for public API

4fc0c11

Refactor plugin configuration

786a35f

Make module export function to meet similar signatures as for PR #99

Update tests for forked ts type checking

5f858d0

Change options parameters for a configuration callback

b20c1a1

Update tests

0613766

Remove extra line in docs blocks

2d669ed

Improve english syntax

8adc7af

davidmpaz added 5 commits July 30, 2017 12:18

Add forked check plugin to dev dependencies

1edae71

Ignore eslint-disable-line rules to satisfay linter. See discussion: #101 (comment)

Fix linting indentation

d2cc270

Update functional test

a8c6d1e

Test throwing Error on wrong configuration

Use const for required variables

87efa63

Fix linting errors

494adf6

weaverryan closed this in 2cae5df Aug 5, 2017

weaverryan mentioned this pull request Aug 5, 2017

Loader features refactor #123

Closed

davidmpaz mentioned this pull request Aug 10, 2017

Add docs for forked type check in webpack-encore symfony/symfony-docs#8275

Closed

davidmpaz deleted the forked-typescript branch August 13, 2017 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forked typescript type checking #101

Forked typescript type checking #101

davidmpaz commented Jul 19, 2017

weaverryan commented Jul 21, 2017

weaverryan Jul 21, 2017

davidmpaz Jul 21, 2017

davidmpaz commented Jul 21, 2017 •

edited

Loading

davidmpaz commented Jul 21, 2017

weaverryan commented Jul 21, 2017

davidmpaz commented Jul 22, 2017

weaverryan left a comment

weaverryan Jul 25, 2017

davidmpaz Jul 25, 2017

weaverryan Jul 27, 2017

weaverryan Jul 25, 2017

weaverryan Jul 25, 2017

weaverryan Jul 25, 2017

davidmpaz Jul 25, 2017

weaverryan Jul 27, 2017

weaverryan Jul 25, 2017

davidmpaz Jul 25, 2017

weaverryan Jul 27, 2017

weaverryan Jul 25, 2017

davidmpaz Jul 25, 2017

weaverryan Jul 27, 2017

weaverryan Jul 25, 2017

weaverryan Jul 25, 2017

davidmpaz Jul 25, 2017

weaverryan Jul 27, 2017

Lyrkan left a comment

Lyrkan Jul 28, 2017

Lyrkan Jul 28, 2017

Lyrkan Jul 28, 2017

weaverryan commented Aug 5, 2017


		'use strict';

		let ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin');

Forked typescript type checking #101

Forked typescript type checking #101

Conversation

davidmpaz commented Jul 19, 2017

weaverryan commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmpaz commented Jul 21, 2017 • edited Loading

davidmpaz commented Jul 21, 2017

weaverryan commented Jul 21, 2017

davidmpaz commented Jul 22, 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

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

Lyrkan 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

weaverryan commented Aug 5, 2017

davidmpaz commented Jul 21, 2017 •

edited

Loading