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

set moduleName to relativePath #173

Closed
wants to merge 1 commit into from

Conversation

patricklx
Copy link
Contributor

@patricklx patricklx commented Jul 5, 2023

set moduleName to relativePath, like its done in ember-cli-htmlbars otherwise it will use the filename, which might be less helpful (and I argue that the filename is not a moduleName, although it can be, but with all the (possible) linking (symbolic, hard link etc) in between I do not think it makes sense), especially when using pnpm the paths will be too long. see emberjs/ember-inspector#2425

@hmajoros
Copy link
Contributor

+1 to this, having the full filepath as moduleName can cause issues between builds via broccoli-caching-writer caching build output if the tree is unchanged. This is problematic if you have a project with multiple addons using workspaces, and build addons independently. Building two dependent addons back to back will cause the second build to use the cached build output from the first, with a now incorrect moduleName.

@asakusuma
Copy link

@ef4 I think this is a better solution to the problem behind emberjs/babel-plugin-ember-template-compilation#13

@patricklx
Copy link
Contributor Author

I'll work on this tomorrow

@patricklx
Copy link
Contributor Author

patricklx commented Jul 20, 2023

I'm still in holidays, so cannot run tests, but i think it was only a linting issue. The tests do not check state in between. Only final html output

@patricklx
Copy link
Contributor Author

patricklx commented Jul 20, 2023

ah just noticed the __tests__ folder... 😅

@patricklx
Copy link
Contributor Author

patricklx commented Jul 20, 2023

It's now ready @ef4 @dfreeman

set moduleName to relativePath, like its done in [ember-cli-htmlbars](https://github.com/ember-cli/ember-cli-htmlbars/blob/master/lib/template-compiler-plugin.js#L22)
otherwise it will use the filename, which might be less helpful, especially when using pnpm the paths will be too long.
@patricklx
Copy link
Contributor Author

@dfreeman failed some scenarios, but errors where unrelated to this pr

@patricklx patricklx closed this Jul 20, 2023
@patricklx patricklx reopened this Jul 20, 2023
@patricklx
Copy link
Contributor Author

@ef4 if content-tag is used, this fix would get lost. I would suggest that content-tag takes also a parameter like moduleName that can be passed as options to template call and then be parsed by babel compiler

@ef4
Copy link
Collaborator

ef4 commented Jul 22, 2023

I'm not sure if that's true, the output of content-tag still goes through babel-plugin-ember-template-compilation. I think that will handle moduleName.

But also: it is fundamentally misleading to use moduleName with template tag anyway, because one module can contain many template tags.

@patricklx
Copy link
Contributor Author

Yes, but the ember template compiler sets it to the filename if nothing is passed....
Better to have something a bit better than that. Although it might not be fully correct

@patricklx
Copy link
Contributor Author

patricklx commented Jul 22, 2023

I do not see where babel-plugin-ember-template-compilation handles it. It would probably also needs to be updated to detect moduleName in the template call params. Or does it pass all other properties to precompileTemplate?

@patricklx
Copy link
Contributor Author

patricklx commented Jul 22, 2023

Maybe i misunderstood, what i meant is that content-tag needs an option to let it know the moduleName, so it can set it to the template params property, then babel-plugin-ember-template-compilation can handle it and pass it to precompileTemplate

@patricklx
Copy link
Contributor Author

@NullVoxPopuli
Copy link
Collaborator

@patricklx can you rebase? I think I deleted one of the files you changed 😅

@patricklx
Copy link
Contributor Author

this would be needed to be done in content-tag now. Closing

@patricklx patricklx closed this Mar 5, 2024
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

5 participants