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

Update definition files in watch mode in webpack@5 #1249

Merged
merged 3 commits into from
Feb 4, 2021

Conversation

appzuka
Copy link
Member

@appzuka appzuka commented Feb 3, 2021

See #1243 for discussion of problem and this fix.

// compilation is actually of type webpack.compilation.Compilation, but afterProcessAssets
// only exists in webpack5 and at the time of writing ts-loader is built using webpack4
const makeAssetsCallback = (compilation: any) => {
compilation.hooks.afterProcessAssets.tap('ts-loader', () =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this code block is only accessible for Webpack 5 you can use the new hook:

compilation.hooks.processAssets.tap({ name: 'ts-loader', stage: webpack.Compilation.PROCESS_ASSETS_STAGE_ADDITIONAL }, (_assets) => {
...
}

Webpack is already required, so no other changes needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

ts-loader is currently built using webpack@4.5.0, which does not define webpack.Compilation.PROCESS_ASSETS_STAGE_ADDITIONAL, so although the code would run under webpack 4 and 5 it cannot be built under webpack4 if we include this.

Also, I think it is OK to use the afterProcessAssets hook. The various processAssets hooks provide a list of the assets that are being processed at that stage, but we are not using the list of assets. We just need a place to hook into where we can emit the assets and afterProcessAssets seems to work fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, please use compilation.hooks.processAssets, you can check webpack version using compiler.webpack (if exists it is webpack@5, if no it is webpack@4), also using compilation.hooks.processAssets I can implement cache, so it will speed build in watch mode

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried building ts-loader using compilation.hooks.processAssets but it will not build because webpack.Compilation.PROCESS_ASSETS_STAGE_ADDITIONAL is not defined in webpack 4. If you try to build using webpack 5 there are other errors. I assume at some point in the future ts-loader will upgrade to webpack 5 but until then we cannot use anything not defined in webpack 4. afterProcessAssets is a valid webpack 5 hook so I'm not sure there is any reason to avoid using it.

Copy link
Contributor

@alexander-akait alexander-akait Feb 4, 2021

Choose a reason for hiding this comment

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

Example of usage https://github.com/webpack-contrib/terser-webpack-plugin/blob/v4.2.3/src/index.js#L647 (webpack@4 and webpack@5), on this (afterProcessAssets) hook I can't help with cache, so if you want speedup your build and do developers happy - OK

Copy link
Contributor

Choose a reason for hiding this comment

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

Just note: maybe to do new major release with dropping webpack@4 is not bad idea

Copy link
Member

Choose a reason for hiding this comment

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

Yeah - I'm in favour

Copy link
Member

Choose a reason for hiding this comment

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

(not in this PR obviously)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I can help with cache and optimizations ⭐

Copy link
Member

Choose a reason for hiding this comment

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

Thanks in advance!

@johnnyreilly
Copy link
Member

Thanks @appzuka - could you update the package.json and CHANGELOG.md please? Also could you respond to @JonWallsten's suggestion?

Cheers!

@johnnyreilly johnnyreilly merged commit b8a70f9 into TypeStrong:master Feb 4, 2021
@johnnyreilly
Copy link
Member

Thanks all - shipping now https://github.com/TypeStrong/ts-loader/releases/tag/v8.0.15

@johnnyreilly
Copy link
Member

I've started work on the webpack 5 migration:

#1251

Help gratefully received! I'm already stuck on a few API renames 😄

@JonWallsten
Copy link
Contributor

@johnnyreilly: I'll do whatever I can to help. How do we coordinate?

@johnnyreilly
Copy link
Member

Let's collaborate on the PR!

This was referenced Mar 11, 2021
This was referenced Mar 14, 2021
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.

4 participants