embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
330 stars 137 forks source link

improve templates moduleName #1517

Closed patricklx closed 10 months ago

patricklx commented 11 months ago

rootDir is not always enough/correct. it ends with /rewritten-app. but there is also /rewritten-packages which will keep the full path. same for node_modules (edit: might have been template imports issue), which have the full path see https://github.com/emberjs/ember-inspector/issues/2425

ef4 commented 10 months ago

Thanks for working on this.

This area is hard to judge because moduleName has a historical meaning that really doesn't make sense anymore. When it was introduced, all packages were forced into one flat namespace and the runtime module namespace had nothing to do with the build-time module namespace. Now neither of those is true.

I think it's probably OK to try to get closer backward compatibility here, just to keep things working. But it has some caveats. It won't really give correct answers for any package that uses exports in package.json to control its own internal layout. And it won't be able to distinguish between multiple versions of the same addon that coexist in the app.

Also if we're going to try to mangle filenames backward to classical moduleNames, there are several cases this doesn't get correct. This implementation will only work for v1 addons (which get rewritten) not v2 addons (which don't). Also, some files that logically live in the app don't physically live in the app, due to classical appTree merging.

I think a full implementation would use PackageCache's ownerOfFile to identify the owning package, and Resolver's reverseSearchAppTree to notice when it really belongs to the app (or some other engine).

If all of that sounds hard, all to get a result which still doesn't reliably tell you where your component came from... yes. That's why I don't think we continue to rely on moduleName and it would be better to spend our time coming up with an alternative API for the inspector that is aligned with how ember & embroider works now.

patricklx commented 10 months ago

Hi, sure and that's okay. I didnt indent to have it full backwards compatible. My issue here is that the moduleNames start at the root of the disk /home/user/project... and not start at the root of the project. at least those in the rewritten packages. It works for rewritten app folder, so root starts in the app folder. It works fine for v2 addon files

patricklx commented 10 months ago

@ef4 any new thoughts on this. I would consider this also as a bugfix. As module name starting root dir sometime starts at the root of the disk

acorncom commented 10 months ago

@patricklx Ed's been pretty busy in the run up to EmberConf, imagine he'll probably take some time off later this week / next week and be in touch after that

patricklx commented 10 months ago

@ef4 this issue still persists for rewritten-packages

ef4 commented 10 months ago

I explained above what a correct implementation of this PR would need to do:

...if we're going to try to mangle filenames backward to classical moduleNames, there are several cases this doesn't get correct. This implementation will only work for v1 addons (which get rewritten) not v2 addons (which don't). Also, some files that logically live in the app don't physically live in the app, due to classical appTree merging.

I think a full implementation would use PackageCache's ownerOfFile to identify the owning package, and Resolver's reverseSearchAppTree to notice when it really belongs to the app (or some other engine).

Please understand that although this PR might happen to solve your exact use case at the moment, (1) it breaks other use cases, (2) it's hard-coded to implementation details that are highly likely to change in the future, like the structure of the rewritten package cache.

I'll accept a fix that goes through the proper APIs to map from a real filename to the logical moduleName.

But also: I really don't want to spend a lot of my time on this feature. I have repeatedly warned people that moduleName is unreliable under modern Ember and they should be moving toward the exits, rather than trying to keep it partially working.

ef4 commented 10 months ago

Is your use case only ember-inspector? Because if so, I'd be happy to try to guide the work to make ember change its output to guide a better inspector implementation.

patricklx commented 10 months ago

Yea, only ember-inspector. Just wanted a bit better ux.

patricklx commented 10 months ago

I'm closing this then

acorncom commented 10 months ago

@patricklx if you're up for, we'd all appreciate your work toward getting things to look sharper in the inspector. Sounds like Ed would happily think about where those changes would be needed ...

patricklx commented 10 months ago

👍 sure, when things are ready, i can make the changes in the inspector.