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: default context should be undefined instead of null #965

Conversation

manuelbieh
Copy link
Contributor

loaderUtils.interpolateName breaks if it passes null to path.relative

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

When css-loader 3.0.0 is used with react-dev-utils/getCSSModuleLocalIdent the build fails because getLocalIdent passes context to loaderUtils.interpolateName. interpolateName then uses this context to get a relative path using path.relative(). While undefined (the value in ^2.1.1) is ... undefined and thus simply ignored by path.relative(), null is an object and causes the error below (shortened):

TypeError [ERR_INVALID_ARG_TYPE]: The "from" argument must be of type string. Received type object
    at C:\htdocs\test\css-loader-issue\test.module.css:1:1
    at validateString (internal/validators.js:107:11)
    at Object.relative (path.js:434:5)
    at Object.interpolateName (C:\htdocs\test\css-loader-issue\node_modules\loader-utils\lib\interpolateName.js:71:10)
    at Object.getLocalIdent (C:\htdocs\test\css-loader-issue\webpack.config.js:22:37)
    at generateScopedName (C:\htdocs\_git\css-loader\dist\utils.js:142:39)
    [...]

This PR removes the context default option completely, implicitly setting its value back to undefined.

Breaking Changes

Existing tests are still passing so I assume there should be no breaking changes. I didn't add any additional tests though.

Additional Info

You should be able to reproduce this behavior by cloning my demo repo and run npm run build.

loaderUtils.interpolateName breaks if it passes null to path.relative
@manuelbieh manuelbieh changed the title fix: context should be undefined instead of null fix: default context should be undefined instead of null Jul 8, 2019
@alexander-akait
Copy link
Member

strange, we use options.context || '' so it should be empty lint and no problems

@manuelbieh
Copy link
Contributor Author

Yes, you do, but only in your internal getLocalIdent() function in utils.js. If a custom getLocalIdent function is passed as option:

modules: { 
  getLocalIdent: (loaderContext, localIdentName, localName, options) => {
    // this should be undefined:
    options.context === null
  }
}

… it's null instead of undefined. So if third-party getLocalIdent functions want to pass context to path.relative() they must take care of checking it for null || '' themselves.

@manuelbieh
Copy link
Contributor Author

… you can check it by adding a console.log(options) to node_modules/loader-utils/lib/interpolateName.js:39. It is:

{ context: null, hashPrefix: '', regExp: null }

in 3.0.0 and

{ regExp: undefined, hashPrefix: '', context: undefined }

in 2.1.1.

context being null instead of undefined causes the interpolateName function to fail because of this line (70):

      directory = path
        .relative(context, resourcePath + '_')

@alexander-akait
Copy link
Member

hm, looks regression, let's add test for this case

@manuelbieh
Copy link
Contributor Author

Just added a test. Please have a look!

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Good job, thanks!

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

2 participants