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

Aliasing/deconflicting overhaul #401

Merged
merged 3 commits into from
Mar 26, 2017
Merged

Aliasing/deconflicting overhaul #401

merged 3 commits into from
Mar 26, 2017

Conversation

Conduitry
Copy link
Member

This was either less hairy than I was expecting, or I missed something. Existing unit tests still pass, a few other manual tests I've run have also passed.

The idea is to make getUniqueName stop colliding with the existing alias function and also with the user's imports. Generator has a new getAliaser mechanism that returns a new aliasing function. Each aliasing function returns identifiers that don't collide with each other (or with preexisting used identifiers, i.e., imports). We're currently making two such aliasers, one that is used for helpers and other top-level scope stuff as before, and the other that is used for getUniqueName which handles lower-scope stuff.

These changes look to be able to cope with a three-way conflict between the template top-level variable, a template import, and the template variable created if you have a component called Template. I do want to get some unit tests written and just have a chance to think through this some more before I remove the [WIP] from the PR.

@Conduitry
Copy link
Member Author

Some other sources of conflicts are:

  • using objects for things that really should be Maps or Sets. This causes problems when things are named the same as things in Object.prototype. I've partially addressed this in the latest commit, but there are still more instances left, and probably yet more that I'm not seeing.
  • names generated by counter that collide with reserved keywords. This could be handled by adding a list of reserved words that the aliasers will try to avoid, but it would probably be simpler to just have counter introduce an underscore or something into all identifiers it produces.

@Conduitry
Copy link
Member Author

Took care of the issue where counter could produce something that was a built-in keyword by having it always prepend the current count, even if it's zero. Took care of another discovered issue where using an eachblock context of key along with keyed updates caused a conflict.

@Conduitry
Copy link
Member Author

One more type of collision I just bumped into - renderer function parameters can shadow various top-level identifiers. Perhaps function parameters should also be run through generator.alias. Not sure how easy that's going to be.

@Conduitry
Copy link
Member Author

I need to take a step back and think about this again. In the meantime I think I'll put together a PR of just the stuff about replacing objects with maps/sets.

@Conduitry
Copy link
Member Author

Updated this again, starting anew from latest master. Quick rundown of what's here -

  • generator.alias is still used for getting consistent names for top-level identifiers (helpers, etc)

  • generator.getUniqueName is still used for getting top-level identifiers that will be different each call

  • generator.current.getUniqueName is still used for getting identifiers specific to a particular renderer function

  • generator.alias is updated to use generator.getUniqueName internally but memoize the results.

  • the .params that are passed down through various functions are now (mostly) being produced by generator.current.getUniqueName

  • many more local variables are having their names also generated by generator.current.getUniqueName.

  • some adjustments to whether various identifiers are being generated by generator.getUniqueName or by generator.current.getUniqueName, as appropriate

  • all the unique name generation functions now should avoid reserved names

Known things that are still not getting deconflicted:

  • an import called root will get shadowed by the root argument to many functions. Renaming the argument seems difficult as the root context is hardcoded a bunch of places in the library
  • having two nested Each blocks with the same named indexing variable creates a function with two parameters with the same name. From a user's point of view, the inner one should probably shadow the outer one. This would probably involve keeping track of indexNames mappings by the index variable rather than by the context name.

@Conduitry Conduitry changed the title [WIP] deconflict getUniqueName from existing aliases and imports Aliasing/deconflicting overhaul Mar 26, 2017
@Conduitry
Copy link
Member Author

I guess I'm ready for some sort of review of this, despite the couple of outstanding things. I've been fiddling with this in a vacuum on and off for a few days now, and it would be nice to get some confirmation that I'm not going totally in the wrong direction here :)

@Rich-Harris
Copy link
Member

This looks good to me — all makes sense, and generally seems like a better/more unified approach. Is it ready for merge?

@Conduitry
Copy link
Member Author

Conduitry commented Mar 26, 2017

It should be. I'd like to write some more unit tests sometime and to refine some stuff a bit, but I don't think that has to hold up this PR.

edit: Actually, with the merging of #407, there might be some stuff from there that has to be updated - the ${local.name}_updating that was introduced there should use the new generated name for that flag, like the other places are.

edit again: Oh! Looks like you already took care of that?

@Conduitry
Copy link
Member Author

Actually, just fixed a small merge issue with the work in #407

@Rich-Harris Rich-Harris merged commit 059fb5f into master Mar 26, 2017
@Rich-Harris Rich-Harris deleted the gh-400 branch March 26, 2017 19:32
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