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

use snake case in generated identifiers (#419) #422

Merged
merged 4 commits into from
Apr 4, 2017
Merged

Conversation

Rich-Harris
Copy link
Member

Ref #419. Figured we may as well create a PR for this branch — will hold off on merging until we resolve this question:

The way that the tmp (formerly __tmp) identifier is now being handled in the branch feels like it might lead to conflicts. Better probably would be to just generate it (with generator.current.getUniqueName) once, store it somewhere on the generator, and then use that in the other places where we need it, similar to how the deconflicted name for component is being handled.

@Conduitry
Copy link
Member

What makes this tmp thing trickier is that the code that uses tmp is generated before the code that declares it. There are three different visitors that generate code that use it, and we don't know which will be called first. I'm not thrilled with it, but what I've pushed to this branch has each place that might use tmp check to see if there's already a name stored on the fragment to use, otherwise it generates one.

@Conduitry
Copy link
Member

Well I was not expecting a unit test failure under Node 4. https://s3.amazonaws.com/archive.travis-ci.org/jobs/216529097/log.txt I don't know what that would be. I guess somehow different places are getting different instances of the fragment, so an update to the tmp property on one of them is not being reflected elsewhere.

@Rich-Harris
Copy link
Member Author

Yikes. I do not understand what is going on with Node 4 at all. Plastered the codebase with console.log statements, been switching between nvm use 4 and nvm use stable, and I am none the wiser.

@Rich-Harris
Copy link
Member Author

Still a bit confused about the discrepancy between Node 4 and 6/7, but the tests all pass now. I reckon we could probably refactor some of the fragment code in future (introduce a Fragment class that contains some of the logic that can handle things like tmp, rather than that logic living in Generator) but that can wait, especially since we may decide to remove tmp in favour of memoizing helpers

@Rich-Harris Rich-Harris merged commit ced1de8 into master Apr 4, 2017
@Rich-Harris Rich-Harris deleted the gh-419 branch April 4, 2017 00:14
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.

2 participants