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

improve templates moduleName #1517

Closed
wants to merge 1 commit into from

Conversation

patricklx
Copy link
Contributor

@patricklx patricklx commented Jul 6, 2023

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 emberjs/ember-inspector#2425

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, which have the full path
see emberjs/ember-inspector#2425
@ef4
Copy link
Contributor

ef4 commented Jul 12, 2023

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
Copy link
Contributor Author

patricklx commented Jul 12, 2023

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
Copy link
Contributor Author

@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
Copy link
Contributor

@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
Copy link
Contributor Author

@ef4 this issue still persists for rewritten-packages

@ef4
Copy link
Contributor

ef4 commented Jul 27, 2023

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
Copy link
Contributor

ef4 commented Jul 27, 2023

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
Copy link
Contributor Author

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

@patricklx
Copy link
Contributor Author

I'm closing this then

@patricklx patricklx closed this Jul 27, 2023
@acorncom
Copy link
Contributor

@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 patricklx deleted the patch-2 branch July 27, 2023 20:42
@patricklx
Copy link
Contributor Author

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

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.

None yet

3 participants