Deprecate Fallback Lookup Paths in ember-resolver #683

gabrielcsapo · 2020-11-23T21:38:44Z

This is a follow up rfc created based on the feedback from issue #661.

rendered text

mydea · 2020-11-24T14:06:00Z

I generally think this is a good idea, my two cents to this:

I believe a replacement for pods needs to be defined before doing that. We use pods for route/controller/templates, and would not like to move those to the classic architecture. See: Colocate route+template+controllers #651 - would love to see some traction on that. If there is interest, I would also write up an RFC PR for that issue based on the ideas collected so far in the issue - it kind of quieted down and there was never any input from the core team on how they view the issue.
I have tried the mentioned ember-strict-resolver and found a lot of problems with it in our app. Not sure if the idea of this RFC is to use that as basis, but if so, I think there is a lot that needs to be done on that front before it could be recommended. Happy to write up my concrete experiences, if that helps in the context of this PR?

gabrielcsapo · 2020-11-24T17:01:54Z

@mydea thanks for your reply!

Totally agree there needs to be a pods replacement, the suggestion for moving forward makes this opt in and since the usage of pods seems constrained to host applications they wouldn't be affected. A suggestion came up when talking to @rwjblue was to have a shim for pods (similar to https://github.com/rwjblue/ember-holy-futuristic-template-namespacing-batman) instead of having that in the main resolver. I think a lot of the issues with pods being supported go away when there is an agreed upon spec for template imports.
We had a similar experience, most of the issues came from our dependencies and our host app not having a consistent service or component lookup pattern. I would like to know what problems you experienced if you could do a writeup that would be great.

mydea · 2020-11-25T07:32:05Z

The main issues I encountered were:

Missing pod support (as mentioned)
Missing support for nested co-location index components (e.g. app/components/my-component/index.js)
Missing pluralization support. Noticed this with ember-can, which has e.g. app/abilitites/my-thing.js, but it wants to look up ability as abilitys. IMHO there should either be support for custom pluralization, or alternatively just do not pluralize unkown types at all - I would be fine with putting these into app/ability/my-thing.js.
After I got nested co-location to work, components from addons using non-colocated structure stopped working for me. Had to manually handle those.
That I had to manually map all multi-word services was also rather unexpected and annoying.

For reference, here is the extended strict resolver that ended up working with our application. There is for sure some room for improvement, but it shows the steps I took to make it work at least:
https://gist.github.com/mydea/d890f3b83a4bf25b3c3a4f41f6a47e42

gabrielcsapo · 2020-12-09T02:25:20Z

Thanks for doing this @mydea

I totally agree the lack of pods support is a known and I think would be great to deprecate pods support in favor of a more mainstream javascript solution to take advantage of open source tools without having to code them to understand the pods architecture.
I was not aware of the nested index components, what would be the benefit of doing this over the normal naming scheme?
Looking at the gist we were able to avoid a lot of the component lookup issues as were already using https://github.com/rwjblue/ember-holy-futuristic-template-namespacing-batman which helped segment our code in in-repo addons very easily.
We had many downstream dependencies that we had to update to ensure that the service names were properly dasherized as a result we made a map and slowly removed items from the map to make the process gradual rather than blocking on it.

I think the biggest advantage of having less logic in the resolver is that it feels less magical. The thing that you want is the thing you get and teaching becomes incredibly straightforward. My favorite part of ember is that there is convention over configuration and this feels very conventionally as we lay out a 1:1 one of doing things instead of a many:1.

mydea · 2020-12-09T07:55:01Z

Having nested components be at app/component/my-component/index can be very helpful when you have a lot of nested components. e.g. Having this:

app/compoments/my-component/index.hbs
app/components/my-component/sub.hbs

Makes it much easier to see all parts belonging together than e.g.

app/components/my-component/sub.hbs
... 50 other folders/components
app/components/my-component.hbs

Regarding the services, not sure I follow. All our services are already referenced in dasherized form, but I still need to map them in the resolver as they are somehow internally (?) looked up in camelcase form.

gabrielcsapo · 2020-12-09T08:05:56Z

@mydea service lookups should not be transformed in the strict resolver case. If you are getting camel case lookups it is coming from having code that doesn't explicitly set the service name in the inject method.

import { inject as service } from "@ember/service";

@service
fooBar

Instead it should be referenced like:

@service("foo-bar")
fooBar

This was hard to track down until we enabled an internal eslint rule that enforced inject calls to ensure a value was provided as a dasherized string.

mydea · 2020-12-09T09:49:40Z

Ah OK, that was absolutely not clear to me, and to be honest also is rather surprising. That kind of feels like just adding more friction to every service definition. I guess it is more explicit, but I haven't seen this as a recommendation/best practice so far.

rwjblue · 2020-12-09T17:54:04Z

That kind of feels like just adding more friction to every service definition

Just to be clear, it is only required for multi-word services.

gabrielcsapo · 2021-01-12T19:12:43Z

@rwjblue is there anyone who would be able to review this?

rwjblue

Overall, I'm definitely in favor of moving in this direction. Thanks for working on this!