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

fix(webpack): fix usage of webpackResolveOptions conditionally #883

Merged

Conversation

jpnelson
Copy link
Contributor

Motivation

There's no existing issue for this as far as I know.

Fixes an issue like this:

INFO ==>  ModuleBuildError: Module build failed (from ./node_modules/thread-loader/dist/cjs.js):
--
INFO ==>  Thread Loader (Worker 6)
INFO ==>  Cannot read property 'extensions' of undefined

Summary

There are some contexts in which the webpackResolveOptions might be undefined – eg issues like TypeStrong/ts-loader#1206 .

In general the loader handles this._compilation not being there, but in practice, breaks because of this line. You can see on line 86 that this is meant to handle the case where webpackResolveOptions is undefined, but it was just missed there (if only we could do it in typescript! But I imagine that's more trouble than it's worth for a webpack loader)

You can compare this with linaria@^2.0.0 to see how it differs in the @linaria/webpack4-loader

Test plan

Tested locally by patching the module with this change

@Anber
Copy link
Collaborator

Anber commented Dec 11, 2021

Hi @jpnelson!

Could you please fix it in webpack5-loader as well?

@@ -59,7 +59,7 @@ export default function webpack4Loader(

// Resolved configuration contains empty list of extensions as a default value
// https://github.com/callstack/linaria/issues/855
if (webpackResolveOptions.extensions?.length === 0) {
if (webpackResolveOptions && webpackResolveOptions.extensions?.length === 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional chaining should help here: webpackResolveOptions?.extensions?.length === 0)

@jpnelson
Copy link
Contributor Author

Good call @Anber , updated now! Thanks

@jeremy-ww
Copy link

Hi @jpnelson, thank you for providing us with such a useful library. And I have encountered the same error there. It's splendid to see a PR issued already before my action. But, I would like to share more information with you guys:

  1. This error occurred to me with the pre-release version @linaria/webpack-loader@3.0.0-beta.16.
  2. Of course, I tried to downgrade it to the pre-release version @linaria/webpack-loader@3.0.0-beta.13.
  3. The error still exists.

Why?

Since the @linaria/webpack-loader@3.0.0-beta.13 is depends on the @linaria/webpack5-loader: ^3.0.0-beta.13. And yarn will always download the latest version of @linaria/webpack5-loader:

image

So I would like to suggest you lock the version without any range modifier or use the distribution tag like next, beta to avoid confusing conditions like that.

Temporarily workaround:

package.json

{
  "resolutions": {
    "**/@linaria/webpack-loader": "3.0.0-beta.13",
    "**/@linaria/webpack5-loader": "3.0.0-beta.13"
  }
}

@jeremy-ww
Copy link

Hi @jpnelson, thank you for providing us with such a useful library. And I have encountered the same error there. It's splendid to see a PR issued already before my action. But, I would like to share more information with you guys:

  1. This error occurred to me with the pre-release version @linaria/webpack-loader@3.0.0-beta.16.
  2. Of course, I tried to downgrade it to the pre-release version @linaria/webpack-loader@3.0.0-beta.13.
  3. The error still exists.

Why?

Since the @linaria/webpack-loader@3.0.0-beta.13 is depends on the @linaria/webpack5-loader: ^3.0.0-beta.13. And yarn will always download the latest version of @linaria/webpack5-loader:

image

So I would like to suggest you lock the version without any range modifier or use the distribution tag like next, beta to avoid confusing conditions like that.

Temporarily workaround:

package.json

{
  "resolutions": {
    "**/@linaria/webpack-loader": "3.0.0-beta.13",
    "**/@linaria/webpack5-loader": "3.0.0-beta.13"
  }
}

Updated: Sorry to ping jpnelson by mistake, it should be @satya164 @Anber. :)

@Anber Anber merged commit 3d6b6c5 into callstack:master Dec 15, 2021
@ntucker
Copy link

ntucker commented Dec 18, 2021

Can we get a release? this is a pretty big bug since webpack 5 doesn't work at all here

@Anber
Copy link
Collaborator

Anber commented Dec 19, 2021

@ntucker I'll publish the next beta later today or tomorrow.

@Anber
Copy link
Collaborator

Anber commented Dec 21, 2021

Hi @ntucker
Sorry for the delay, but critical issue #891 has been found and should be fixed before the release.

@selmalee
Copy link

selmalee commented Jan 23, 2022

When will this fix be released please?

@wmzy
Copy link
Contributor

wmzy commented May 31, 2022

@Anber could you publish this to latest tag?

@Anber
Copy link
Collaborator

Anber commented Jun 3, 2022

@wmzy done!

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.

None yet

6 participants