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

Behaviour of the "[path]" placeholder have been changed in a refactor commit #967

Closed
fa93hws opened this issue Jul 15, 2019 · 17 comments · Fixed by #972
Closed

Behaviour of the "[path]" placeholder have been changed in a refactor commit #967

fa93hws opened this issue Jul 15, 2019 · 17 comments · Fixed by #972

Comments

@fa93hws
Copy link

fa93hws commented Jul 15, 2019

  • Operating System:
  • Node Version:
  • NPM Version:
  • webpack Version:
  • css-loader Version: 3.0+

Expected Behavior

[path] placeholder gives path-to-file

Actual Behavior

[path] placeholder gives path/to/file

How Do We Reproduce?

Should be very clear in this commit. #920
normalizeIdentifier is removed.

@alexander-akait
Copy link
Member

Please create minimum reproducible test repo

@fa93hws
Copy link
Author

fa93hws commented Jul 15, 2019

Please create minimum reproducible test repo

It should be clear enough by seeing here https://github.com/webpack-contrib/css-loader/pull/920/files#diff-2b4ca49d4bb0a774c4d4c1672d7aa781L108.

If you are looking for the config

    modules: {
      mode: 'local',
      context: path.resolve(__dirname, '../src'),
      localIdentName: '[path][name]__[local]',
    },
    localsConvention: 'camelCaseOnly',
    importLoaders: 0,
sourceMap: true,

If you are looking for the repo
https://github.com/fa93hws/simple-typescript-react-starter

@alexander-akait
Copy link
Member

alexander-akait commented Jul 15, 2019

Still can't understand, what is output was previously and what is output actually, no need link on source code, just clarify your problem

@fa93hws
Copy link
Author

fa93hws commented Jul 15, 2019

Still can't understand, what is output was previously and what is output actually, no need link on source code, just clarify your problem

The [path] in classname was mapped to path-to-file and it is mapped to path/to/file now.

@alexander-akait
Copy link
Member

@fa93hws looks like regression, feel free to send a PR

@fa93hws
Copy link
Author

fa93hws commented Jul 15, 2019

@fa93hws looks like regression, feel free to send a PR

No problem. OK to have this function back? https://github.com/webpack-contrib/css-loader/pull/920/files#diff-2b4ca49d4bb0a774c4d4c1672d7aa781L108

@alexander-akait
Copy link
Member

alexander-akait commented Jul 15, 2019

No, css classes can have non standard characters, so we can't back this function

@alexander-akait
Copy link
Member

stop, why you think it is bug? Looks like it is expected behavior, and looks it is breaking change, i think you should use own localIdentName implementation if you want to have -

@fa93hws
Copy link
Author

fa93hws commented Jul 15, 2019

stop, why you think it is bug? Looks like it is expected behavior, and looks it is breaking change, i think you should use own localIdentName implementation if you want to have -

Because that commit says it's a "refactor" and a "refactor" normally won't change the behavior of the code.
That make me think it's probably a mistake?

@fa93hws
Copy link
Author

fa93hws commented Jul 15, 2019

stop, why you think it is bug? Looks like it is expected behavior, and looks it is breaking change, i think you should use own localIdentName implementation if you want to have -

Anyway, if it is expected behavior, I am happy to implement the - in my configuration.
Thanks

@fa93hws fa93hws closed this as completed Jul 15, 2019
@alexander-akait
Copy link
Member

alexander-akait commented Jul 15, 2019

Sometimes refactor has breaking change. Css allow to use / character so we need allow to use them in code also, but [path][name]__[local] to /path/to/name__hash looks very bad and it is very uncomfortably, maybe we should change / to - only for placeholders for better DX

@alexander-akait
Copy link
Member

Let's think about this, because maybe it is really not good DX

@alexander-akait
Copy link
Member

In version@2 it was -, right (i am look on code)?

@fa93hws
Copy link
Author

fa93hws commented Jul 15, 2019

In version@2 it was -, right (i am look on code)?

2.some.version is with - but 3.0 is not.
I'm not sure at which version it starts not to replace the / but it's after this PR: #920

I will replace the '/' to '-' only then.

@alexander-akait
Copy link
Member

Let's keep open i think about this in near future, as workaround you can use custom localIdentName function to fix problem, feel free to feedback. Here interesting problem

@fa93hws
Copy link
Author

fa93hws commented Jul 15, 2019

Let's keep open i think about this in near future, as workaround you can use custom localIdentName function to fix problem, feel free to feedback. Here interesting problem

No problem, Thanks

@alexander-akait
Copy link
Member

Yes, it is big regression 😞 thanks for catching this

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 a pull request may close this issue.

2 participants