Deprecate Fallback Lookup Paths in ember-resolver

[gabrielcsapo]

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

rendered text

[mydea]

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: https://github.com/emberjs/rfcs/issues/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]

@mydea thanks for your reply!

1. 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.
2. 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]

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]

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]

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]

@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]

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]

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]

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

[wagenet]

I'm going to try to bring this up with the core team and see if we can move it to a conclusion.

[wagenet]

@chriskrycho should this be part of #832?

[chriskrycho]

@wagenet I don’t think so; this is probably still worth doing if @gabrielcsapo wants to drive it forward, but separately from the Classic stuff. The resolver has its own path to slowly being replaced, but is separate from Classic issues.

[achambers]

@ef4 Is this in any way related to the RFCs you're currently working on? Also seems Embroider related. Do you have any thoughts to add here?

[ef4]

Yes, https://github.com/emberjs/rfcs/pull/938 wants to introduce a new resolver implementation. That makes it a good time to drop deprecated resolver features. So if this RFC can land, we can make the new resolver in 938 not support the things this RFC deprecates.

One bit of feedback for this RFC is that we can probably avoid using an optional feature, in favor of making ember-resolver a V2 addon and having dedicated import paths for the different implementations.

[achambers]

@ef4 Just to clarify, if the new resolver in #938 doesn't implement the things that this RFC (#683) deprecates, then 683 is not needed, correct?

[ef4]

Well, the work of deciding which things not to include still needs to happen. If #683 covers that work it makes it easier to do #938.

[wagenet]

We agreed to move this to exploring at the RFC Review Meeting.