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

Fixed missing errors in watch mode in webpack5 #1208

Merged
merged 2 commits into from
Nov 7, 2020
Merged

Fixed missing errors in watch mode in webpack5 #1208

merged 2 commits into from
Nov 7, 2020

Conversation

appzuka
Copy link
Member

@appzuka appzuka commented Nov 6, 2020

The fix in PR1200 was made to prevent deprecation warnings with webpack5, because it is no longer allowed to add assets during the afterCompile hook. These functions were moved to the new afterProcessAssets hook in webpack5. It seems that in watch mode, the afterProcessAssets hook is not called, so if any errors are introduced they will not be reported to webpack.

This PR allows the makeAfterCompile function to select whether assets should be added and/or errors should be reported. Running under webpack4 the behaviour is unchanged. Under webpack5 assets are added during afterProcessAssets and errors are reported during afterCompile. This should correct the behaviour observed in #1204.

This PR passes all comparison and execution tests, but as these run under webpack4 the webpack5 behaviour is not tested. I assume this will be corrected in the future when webpack5 is used to build ts-loader.

src/instances.ts Outdated
// Adding assets in afterCompile is deprecated in webpack 5 so we
// need different behavior for webpack4 and 5

if (!!webpack.version!.match(/^4.*/)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should do caching here for perf reasons? The webpack version value won't change so regex-ing each time is perhaps less performant than if we cached it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only run once per compile cycle so I'm not sure how much difference it makes. I understand javascript regexes are not fast but I'm not sure if the impact is significant here. We could eliminate the regex by using

if (!loader._compilation.hooks.afterProcessAssets)

instead, as this does not exist in webpack4, but I thought checking webpack.version makes the code easier to understand. I assume that in the medium term there will be a webpack5 version of ts-loader so this is probably not a long term issue.

If you think caching is worthwhile I'll update the PR.

Copy link
Member

@johnnyreilly johnnyreilly Nov 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as an alternative to testing each time what do you think about doing something like this? (determining the function created based upon the webpack version) - this way we determine it once and we get the exact function we need each time:

// Adding assets in afterCompile is deprecated in webpack 5 so we
// need different behavior for webpack4 and 5
const addAssetHooks = !!webpack.version!.match(/^4.*/) ? (
  loader: webpack.loader.LoaderContext,
  instance: TSInstance
) => {
    // add makeAfterCompile with addAssets = true to emit assets and report errors
    loader._compiler.hooks.afterCompile.tapAsync(
      'ts-loader',
      makeAfterCompile(instance, true, true, instance.configFilePath)
    );
}
: (
  loader: webpack.loader.LoaderContext,
  instance: TSInstance
) => {
  // We must be running under webpack 5+

  // Add makeAfterCompile with addAssets = false to suppress emitting assets
  // during the afterCompile stage. Errors will be still be reported to webpack
  loader._compiler.hooks.afterCompile.tapAsync(
    'ts-loader',
    makeAfterCompile(instance, false, true, instance.configFilePath)
  );

  // Emit the assets at the afterProcessAssets stage
  loader._compilation.hooks.afterProcessAssets.tap('ts-loader', (_: any) => {
    makeAfterCompile(
      instance,
      true,
      false,
      instance.configFilePath
    )(loader._compilation, () => {
      return null;
    });
  });

  // It may be better to add assets at the processAssets stage (https://webpack.js.org/api/compilation-hooks/#processassets)
  // This requires Compilation.PROCESS_ASSETS_STAGE_ADDITIONAL, which does not exist in webpack4
  // Consider changing this when ts-loader is built using webpack5
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a neat solution. Are you going to make the change or do you want me to update the PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not on my computer right now - so if you're able to do it that'd be amazing! But yeah - I'd be happy to do at some time over the weekend if you're busy. Happy either way

@johnnyreilly
Copy link
Member

This is a tremendous PR! Great documentation in the code - it's really helpful. I've one question but otherwise I think this is marvellous!

@johnnyreilly johnnyreilly merged commit 4909d99 into TypeStrong:master Nov 7, 2020
@johnnyreilly
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants