feat: support for noEmitOnErrors property in webpack.config #337

Fatme · 2019-09-25T10:10:34Z

Currently noEmitOnErrors property is not respected from webpack compilation in case it is set in webpack.config.js. As a result, the files are emitted even when there is an error in the source code. However, it turns out that ts-loader is working correctly in transpileOnly: false mode and the problem is only in case when ts-loader is started in transpileOnly: true + ForkTsCheckerWebpackPlugin.

After investigating the issue, it turns out that the problem is here. Webpack has a logic that checks if the files should be emitted based on compilation.errors array. This logic is executed on shouldEmit hook as can be seen here. So, we need to put our reported diagnostics and lints in compilation.errors before shouldEmit.

The afterCompile hook seems as a perfect solution as it is executed before shouldEmit and is an async hook. As supporting this feature is important for our error handling story, we implemented the change, spent a little time testing it and it works for our cases.

However, we're not absolutely sure for the impact of the mentioned change so we'd be glad to hear your opinion and suggestions.

Rel to: #57

Webpack doesn't notify NativeScript CLI when there is a compilation error. So, NativeScript CLI build the app, install it on device and start it. In most cases the application crashes runtime as the webpack compilcation is not successful. Most probably there would be a red/yellow message in the console printed during the compilation. The users don't see the error message as there is too much log outputed in the console. They don't understand the exact reason why their app crashes runtime. Webpack has a mechanism to skip the emitting phase whenever there are errors while compiling. This can be achieved using `optimization.noEmitOnErrors` property as it is described [here](https://webpack.js.org/configuration/optimization/#optimizationnoemitonerrors). This PR adds `noEmitOnErrors` property in all webpack.config files: * The default value is based on `noEmitOnError` property from `tsconfig.json` for `angular` and `typescript` projects * The default value is `true` for `javascript` and `vue` projects. Also this PR fixes the following problems: 1. Check for syntactic errors when running webpack compilation in ts projects Currently `ts-loader` is started in `transpileOnly` mode and webpack plugin (`ForkTsCheckerWebpackPlugin`) runs TypeScript type checker on a separate process in order to report for compilation errors. By default the plugin only checks for semantic errors and adds them to `compilation.errors` as can be seen [here](). On the other side, webpack relies on [compilation.errors]() array when deciding if should skip emitting phase. However, `ts-loader` used in `transpileOnly` mode still reports syntactic errors but adds them to `compilation.warnings`. This is a problem, as actually the compilation is not stopped when there is a syntactic error. Setting `checkSyntacticErrors: true` will ensure that `ForkTsCheckerWebpackPlugin` will check for both syntactic and semantic errors and after that will be added to `compilation.errors`. 2. Respect `noEmitOnError` from `tsconfig.json` when compiling in `ts` projects The problem is in `ForkTsCheckerWebpackPlugin` and in the way it is integrated with webpack hooks - TypeStrong/fork-ts-checker-webpack-plugin#337. 3. Send the hash of compilation to NativeScript CLI The `hmr` generates new hot-update files on every change and the hash of the next hmr update is written inside hot-update.json file. Although webpack doesn't emit any files, hmr hash is still generated. The hash is generated per compilation no matter if files will be emitted or not. This way, the first successful compilation after fixing the compilation error generates a hash that is not the same as the one expected in the latest emitted hot-update.json file. As a result, the hmr chain is broken and the changes are not applied. Rel to: NativeScript/nativescript-cli#3785

piotr-oles · 2019-10-13T10:38:55Z

Thank you for your findings and for the PR :)
Unfortunately, changing the hook from emit to afterCompile is a breaking change in my opinion. There is a custom hook: fork-ts-checker-emit - and as it's a part of public API, some developers could assume that it will run on the emit event.
Because of that, we need to find a method to deprecate the fork-ts-checker-emit hook and add a new one (fork-ts-checker-after-compile) + keep default behavior of using webpack's emit event (probably we will have to add a new configuration option to control that behavior).

I know that it can be frustrating that we cannot just fix this bug and change the hook, but our responsibility is to introduce breaking changes gracefully :)

Fatme · 2019-10-23T19:15:52Z

Hey @piotr-oles,

Thank you very much for taking a look at this PR.

but our responsibility is to introduce breaking changes gracefully

I totally agree that we have to ensure seamless transition for the users.

keep default behavior of using webpack's emit event (probably we will have to add a new configuration option to control that behavior)

Yep, maybe introducing a new configuration option is a good solution. Having that option with default value false will ensure the new version of the plugin will not break existing users and applications. So we need a name for the new option 😸 - maybe useAfterCompile or noEmitOnErrors or something else? I'm opened to any ideas and suggestions.

As this PR is really important for us and our error handling story, I'll prepare the needed changes once we have an agreement about the name of the new option.

piotr-oles · 2019-10-23T19:19:29Z

I would suggest useAfterCompileHook, @johnnyreilly, what do you think?

johnnyreilly · 2019-10-23T20:58:58Z

We could do... I'm just wondering if it might make more sense to do a breaking changes version? After all, do we want to maintain two behaviours in the long term? If we all agree this is a more sensible behaviour then having a breaking changes version seems like a way to not break existing users and move the plugin in the direction we want to go... Thoughts?

Fatme · 2019-10-24T10:06:46Z

Hey @piotr-oles, @johnnyreilly

It seems we have 2 options:

Release a breaking change version of the plugin - If we agree on that, I suppose it'll be released as a major version e.g 2.0.0.
Add a configuration option and support the both behaviors.

The both solutions will work for us and will address our problem. However, personally to me, I prefer releasing a major version and introducing it as a breaking change. That will be the most beneficial solution in the long term.

Let me know about your decision, so I can apply the required changes on the PR.

piotr-oles · 2019-10-24T10:20:34Z

Ok, maybe breaking change is a better option - it will probably not break anything for 99% of users.

Please update the documentation (Plugin hooks section) and add BREAKING CHANGE section to the commit message :)

Fatme · 2019-10-24T11:14:12Z

@piotr-oles,

I changed the commit message with BREAKING CHANGE: footer and updated the plugin section in the documentation.

Let me know if I've missed something or there is something else that I can do.

johnnyreilly · 2019-10-24T14:51:03Z

Could you catch up this branch with master please?

Fatme · 2019-10-24T17:00:12Z

@piotr-oles, @johnnyreilly,

I've rebased the PR on the latest master branch.

johnnyreilly

Looks good to me!

Fatme · 2019-10-24T18:27:52Z

@johnnyreilly, @piotr-oles,

It seems there are failing CI builds https://travis-ci.org/TypeStrong/fork-ts-checker-webpack-plugin/jobs/602424646?utm_medium=notification&utm_source=github_status:

error An unexpected error occurred: "https://registry.yarnpkg.com/babel-code-frame/-/babel-code-frame-6.26.0.tgz: Request failed \"503 Service Unavailable\"".
info If you think this is a bug, please open a bug report with the information provided in "/home/travis/build/TypeStrong/fork-ts-checker-webpack-plugin/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
The command "yarn install --frozen-lockfile" failed and exited with 1 during .

Maybe we should rerun the travis builds.

johnnyreilly · 2019-10-24T18:31:37Z

Retriggered. Travis has been pretty flaky this week

Fatme · 2019-10-25T05:29:30Z

@johnnyreilly, @piotr-oles

We have a green build! 🎉

Do I need to rebase it again on master as there are additional commits merged into master or you can merge the PR directly?

johnnyreilly · 2019-10-25T05:55:16Z

Yes please - update the branch!

The afterCompile hook should be blocked instead of emit in order to handle correctly noEmitOnErrors property from webpack.config.js BREAKING CHANGE: The emit hook is replaced with afterCompile hook.

Fatme · 2019-10-25T07:32:59Z

@piotr-oles, @johnnyreilly,

Thank you very much for merging the PR!
When do you plan to release it officially?

piotr-oles · 2019-10-25T09:28:44Z

It will be released automatically by 📦 🚀 semantic-release when CI build finish :)

Fatme · 2019-10-25T12:35:01Z

@piotr-oles,

What about the version - will it be updated to 2.0.0 as we're releasing a breaking change?

piotr-oles · 2019-10-25T12:38:12Z

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

piotr-oles · 2019-10-25T12:42:48Z

The CI is performing very badly last time - I had to re-run the release job. And yes - as you can see semantic-release do all the magic ✨ 😃

Fatme · 2019-10-25T12:43:45Z

Yeep, it's really cool magic! 😃

We have a limitation that webpack compilation doesn't stop on error in typescript applications. That was due to the issue in fork-ts-checker-webpack-plugin. After merging [the PR that fixes the issue](TypeStrong/fork-ts-checker-webpack-plugin#337) and releasing 2.0.0 version of the plugin, we can update it on our side and that way webpack compilation will stop on syntax/semantic errors within the application. Rel to: NativeScript/nativescript-cli#3785

Fatme mentioned this pull request Sep 25, 2019

Suport for noEmitOnError? #57

Closed

Fatme mentioned this pull request Sep 26, 2019

fix: handle correctly webpack compilation errorrs NativeScript/nativescript-dev-webpack#1051

Merged

5 tasks

Fatme force-pushed the fatme/no-emit-on-errors branch from 10273e7 to 83112ba Compare October 24, 2019 11:12

Fatme force-pushed the fatme/no-emit-on-errors branch from 83112ba to a685243 Compare October 24, 2019 16:58

johnnyreilly previously approved these changes Oct 24, 2019

View reviewed changes

Fatme added 3 commits October 25, 2019 09:04

fix: replace emit hook with afterCompile

15c3bfc

The afterCompile hook should be blocked instead of emit in order to handle correctly noEmitOnErrors property from webpack.config.js BREAKING CHANGE: The emit hook is replaced with afterCompile hook.

refactor: rename emitCallback to afterCompileCallback

6675083

fix: change fork-ts-checker-emit to fork-ts-checker-after-compile

4562102

Fatme dismissed johnnyreilly’s stale review via 4562102 October 25, 2019 06:04

Fatme force-pushed the fatme/no-emit-on-errors branch from a685243 to 4562102 Compare October 25, 2019 06:04

johnnyreilly approved these changes Oct 25, 2019

View reviewed changes

piotr-oles approved these changes Oct 25, 2019

View reviewed changes

piotr-oles merged commit dd410b0 into TypeStrong:master Oct 25, 2019

piotr-oles added the released label Oct 25, 2019

Fatme mentioned this pull request Oct 25, 2019

fix: stop on compilation error in typescript applications NativeScript/nativescript-dev-webpack#1078

Merged

5 tasks

johnnyreilly mentioned this pull request Oct 28, 2019

v2.0.0 breaks async: false #351

Closed

Uh oh!

feat: support for noEmitOnErrors property in webpack.config #337

feat: support for noEmitOnErrors property in webpack.config #337

Uh oh!

Conversation

Fatme commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piotr-oles commented Oct 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fatme commented Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piotr-oles commented Oct 23, 2019

Uh oh!

johnnyreilly commented Oct 23, 2019

Uh oh!

Fatme commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piotr-oles commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fatme commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnnyreilly commented Oct 24, 2019

Uh oh!

Fatme commented Oct 24, 2019

Uh oh!

johnnyreilly left a comment

Choose a reason for hiding this comment

Uh oh!

Fatme commented Oct 24, 2019

Uh oh!

johnnyreilly commented Oct 24, 2019

Uh oh!

Fatme commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnnyreilly commented Oct 25, 2019

Uh oh!

Fatme commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piotr-oles commented Oct 25, 2019

Uh oh!

Fatme commented Oct 25, 2019

Uh oh!

piotr-oles commented Oct 25, 2019

Uh oh!

piotr-oles commented Oct 25, 2019

Uh oh!

Fatme commented Oct 25, 2019

Uh oh!

Uh oh!

Fatme commented Sep 25, 2019 •

edited

Loading

piotr-oles commented Oct 13, 2019 •

edited

Loading

Fatme commented Oct 23, 2019 •

edited

Loading

Fatme commented Oct 24, 2019 •

edited

Loading

piotr-oles commented Oct 24, 2019 •

edited

Loading

Fatme commented Oct 24, 2019 •

edited

Loading

Fatme commented Oct 25, 2019 •

edited

Loading

Fatme commented Oct 25, 2019 •

edited

Loading