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

Support Modifiers #44

Closed
wants to merge 12 commits into from
Closed

Support Modifiers #44

wants to merge 12 commits into from

Conversation

NullVoxPopuli
Copy link
Collaborator

Resolves #42

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review October 28, 2019 15:19
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, we should not propagate the idea that there are separate namespaces in handlebars files. We should assume there is a single unified list of available "invokable" things, which is made up of all helpers, modifiers, and components.

tldr; I don't think we need to track the first path expression within a modifier 😄

@NullVoxPopuli
Copy link
Collaborator Author

we should not propagate the idea that there are separate namespaces in handlebars files

what do you mean by this / how does this PR have that idea?

We should assume there is a single unified list of available "invokable" things

I almost did that approach, the same way helpers and computeds are done right now, but found that the telemetry-helpers' default info collector doesn't do modifiers, so I came up with this approach.

I don't think we need to track the first path expression within a modifier

Tobias was saying the right thing to do would be to add some behavior to glimmer/syntax so that Path nodes can know what they are / have consumers of the visitor pattern not have to do what's being done in this codemod with enter/exit 🤷‍♂️
once that exists, I'm happy to delete code. :D

@rwjblue
Copy link
Member

rwjblue commented Oct 28, 2019

found that the telemetry-helpers' default info collector doesn't do modifiers

We should fix it!

@rwjblue
Copy link
Member

rwjblue commented Oct 28, 2019

Tobias was saying the right thing to do would be to add some behavior to glimmer/syntax so that Path nodes can know what they are / have consumers of the visitor pattern not have to do what's being done in this codemod with enter/exit 🤷‍♂

Ya, totally agree. I think that work would be great, but I don't think we need that for this specific PR.

@NullVoxPopuli
Copy link
Collaborator Author

This passes locally, so maybe the failure is cache related?

@NullVoxPopuli
Copy link
Collaborator Author

@tylerturdenpants any chance you could take a gander at this? I'm confused why it seems like the test running on 3.13 doesn't match my machine -- so, I wonder if maybe my machine is somehow broken to the point of "working", and the broken state in CI is correct.

@tylerturdenpants
Copy link
Contributor

Ya. I’ll take a look at it today. Going to work on a few telemetry related things.

@Turbo87
Copy link
Contributor

Turbo87 commented Dec 14, 2019

I guess we can close this now, since #62 should've added support for modifiers 🎉

@NullVoxPopuli NullVoxPopuli deleted the support-modifiers branch December 15, 2019 04:51
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.

Render modifiers prefixed with this.
4 participants