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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix name colission: use full path instead of base #147

Closed
wants to merge 1 commit into from

Conversation

viankakrisna
Copy link
Contributor

path.Base only returns the last element of path https://golang.org/pkg/path/#Base

so if you have

someModule/action.js

otherModule/action.js

variable name for both modules gonna be

require_action_exports

which is causing collission because each module live on the same scope now

this change would need us to update all the tests, do you have a way to update it programmatically?

we probably could use the path from cwd instead full path too so it's easier to read 馃

@evanw
Copy link
Owner

evanw commented May 27, 2020

This function is called GenerateNonUniqueNameFromPath() because it shouldn't need to be unique. Name collisions are serious bugs, and should be fixed in the renamer instead. Is there an actual name collision in the generated code? Or is this more of an aesthetic improvement?

Aesthetically, a related problem that I've noticed is that there are a lot of functions named similar to require_index() from packages in node_modules that use node's implicit index.js path resolution behavior. It'd be good to fix that too.

I'm also worried about names getting to be too long. I bet just using the last two path components (immediate parent directory and file) would be a good compromise here.

There currently isn't a way to programmatically update tests. The test failures on this PR seem to demonstrate a regression in readability. It looks to me like export objects that used to have alphabetic identifiers now are named _ instead.

@viankakrisna
Copy link
Contributor Author

@evanw yes, there's an actual name colission in the generated bundle, i haven't yet have the time to extract this to a reduced test / example with the config i use in my project, but this change make the colission gone. so there's probably some code path that we should not call GenerateNonUniqueNameFromPath. i'll try to figure out what exactly happening here

@evanw
Copy link
Owner

evanw commented Jun 9, 2020

Someone else reported a name collision that sounds like the same issue: #174. They provided a test case and I was able to fix it, so I think the problem this PR is trying to address no longer exists.

@evanw evanw closed this Jun 9, 2020
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