Rewrite PathExpression visitor code

[Turbo87]

Instead of having a single visitor for PathExpression it is more useful to know if it is the path of a simple mustache, a block, a modifier, etc.

Depending on the parent element we can assume certain things, e.g. inside an AttrNode the mustache path can't be a component invocation.

This rewrite also fixes the wrong priority order where properties had priority over components and helpers, which is different in an actual app. If a component/helper with a corresponding name is found than the prefixing will be skipped. This removes the reliance on getTelemetryFor() which helps bring us closer to "pods" support.

Due to the different implementation this also removes the dontAssumeThis option from the codemod.

[NullVoxPopuli]

why remove this behavior/test?

[Turbo87]

a path can only be either:

• a property
• a scoped variable
• a component invocation
• a helper invocation

if it is not a component/helper invocation and not a scoped variable then there is only "property" left. if we use dontAssumeThis: true in what case would we then prefix the path with this.?

[NullVoxPopuli]

good point. that would be confusing. I think I'll have to do a major bump, just in case though.

[Turbo87]

I think I'll have to do a major bump, just in case though.

yeah, totally agree. I'd wait a bit though so that people can try it out from master, and I might have a few more PRs coming in... 😅

[NullVoxPopuli]

I'd wait a bit though

too late!
I have no problem with multiple releases or multiple breaking releases even.
release-it is my favorite tool for this stuff. so easy.

[patocallaghan]

So we were actually using this functionality 😬 but if it's not deemed useful then 🔪

For our use case, while we know that passed in args are reflected onto a classic component and all its properties can be referenced using this., we actually didn't want to do just migrate all properties in our templates to this.. We wanted to try and semantically migrate our templates to properly disambiguate between this. vs @ properties.

Our process for this was

1. Run this codemod on said template
2. Turn on no-implicit-this ember-template-lint rule
3. Any properties which didn't have this. prepended to them we'd manually fix on a case-by-case basis depending on whether the property existing in the JS (i.e. this.set('someProperty', ...)) or just passed directly in.

[NullVoxPopuli]

I think that's def a valid workflow, and should maybe be called out in the readme on this repo. @patocallaghan (or really anyone), a PR to add don't-assume-this back in would be worthwhile.

however, I keep forgetting, if you don't-assume-this, does the codemod do anything?

[Turbo87]

no, in that case the codemod would literally do nothing, so I'm not sure if it's actually that useful 🤔

[patocallaghan]

So when we say it literally did nothing, before the default behaviour was always to append this. but with this option it just opted you out of that and it left the property as is if it didn't find it in the component's telemetry. It was then on you to manually figure out if it was this. or @.

[NullVoxPopuli]

that makes sense. We've had some issues with getting property information in telemetry from all the different types of classes / ember versions / ways of constructing "Meta". :(

[NullVoxPopuli]

Looks good to me, just a couple questions.