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

feat: add hashSeed config in babel-plugin-react-scoped-css plugin #28

Merged

Conversation

BakuJun
Copy link
Contributor

@BakuJun BakuJun commented Jun 23, 2020

Thank you for your plugin.
When I use this plugin in multiple projects(pubish as npm),I found the same file name in different projects data-v-md5(filename) is conflicted.
So I provide a MR with filePath to md5 ,to avoid this situation.

@gaoxiaoliangz
Copy link
Owner

I have thought about this before, the reason why I used filename is because that I want consistent hash in CI and local environments, so that snapshot tests will not fail. Maybe it's better to use project-level relative path, but in the context of babel I cannot determine the project absolute path. Therefore I cannot get project-level relative path. Maybe there is a way, but I did not go further.

@BakuJun
Copy link
Contributor Author

BakuJun commented Jun 28, 2020

I have thought about this before, the reason why I used filename is because that I want consistent hash in CI and local environments, so that snapshot tests will not fail. Maybe it's better to use project-level relative path, but in the context of babel I cannot determine the project absolute path. Therefore I cannot get project-level relative path. Maybe there is a way, but I did not go further.

How about use

const cwdName = getFilenameFromPath(process.cwd());
const filename = getFilenameFromPath(path.node.source.value);

const hash = md5(cwdName + filename + lastHash).substr(0, 8)

cwdName is closer to project name.

@gaoxiaoliangz
Copy link
Owner

As a babel plugin, the user may use it in in all kinds of situations, and it's possible that cwd is not the project root. But if CI and local use the same cwd it maybe fine. So I think we can use cwd if there is no better option. And as we are using cwd, we can actually use path.relative to resolve the project relative path. Hash can be generated using that with more info thus reducing the risk of collision.

@BakuJun
Copy link
Contributor Author

BakuJun commented Jun 28, 2020

As a babel plugin, the user may use it in in all kinds of situations, and it's possible that cwd is not the project root. But if CI and local use the same cwd it maybe fine. So I think we can use cwd if there is no better option. And as we are using cwd, we can actually use path.relative to resolve the project relative path. Hash can be generated using that with more info thus reducing the risk of collision.

Relative path has the same problem.For example, I have two projects npmA and npmB. My entry file's relative path is src/index.tsx

const relative = path.relative(process.cwd(),filePath); //  value is 'src/index.tsx'

Relative path lost project name, but the cwd path works. I have updated MR and test case.Please have a look.

@BakuJun BakuJun changed the title fix: use absolute path to md5, avoid className conflict in multiple p… fix: use cwd name to md5, avoid className conflict in multiple p… Jun 28, 2020
@gaoxiaoliangz
Copy link
Owner

I get your point, but using cwd folder name has the potential of generating different hashes on different machines, 'cause project folder names can be different.

I have another idea. Maybe we can let the user provide something like a hashSeed as a babel plugin option, and compute hash like this. And this hashSeed can be optional.

const hash = md5(hashSeed + fileRelativePath + lastHash)

@BakuJun
Copy link
Contributor Author

BakuJun commented Jun 28, 2020

I get your point, but using cwd folder name has the potential of generating different hashes on different machines, 'cause project folder names can be different.

I have another idea. Maybe we can let the user provide something like a hashSeed as a babel plugin option, and compute hash like this. And this hashSeed can be optional.

const hash = md5(hashSeed + fileRelativePath + lastHash)

HashSeed is a good idea. Please check the new commit.

@BakuJun BakuJun changed the title fix: use cwd name to md5, avoid className conflict in multiple p… feat: add hashseed confi ing babel-plugin-react-scoped-css Jun 28, 2020
@BakuJun BakuJun changed the title feat: add hashseed confi ing babel-plugin-react-scoped-css feat: add hashseed config in babel-plugin-react-scoped-css plugin Jun 28, 2020
@BakuJun BakuJun changed the title feat: add hashseed config in babel-plugin-react-scoped-css plugin feat: add hashSeed config in babel-plugin-react-scoped-css plugin Jun 28, 2020
@gaoxiaoliangz gaoxiaoliangz merged commit 8fbefc5 into gaoxiaoliangz:master Jun 28, 2020
@gaoxiaoliangz
Copy link
Owner

Thanks for the PR. I'll release this update soon.

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