Skip to content

set moduleName to relativePath - #173

Closed
patricklx wants to merge 1 commit into
ember-cli:masterfrom
patricklx:patch-1
Closed

set moduleName to relativePath#173
patricklx wants to merge 1 commit into
ember-cli:masterfrom
patricklx:patch-1

Conversation

@patricklx

@patricklx patricklx commented Jul 5, 2023

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown

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

@patricklx

Copy link
Copy Markdown
Contributor Author

I'll work on this tomorrow

@patricklx

patricklx commented Jul 20, 2023

Copy link
Copy Markdown
Contributor Author

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

patricklx commented Jul 20, 2023

Copy link
Copy Markdown
Contributor Author

ah just noticed the __tests__ folder... 😅

@patricklx

patricklx commented Jul 20, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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

ef4 commented Jul 22, 2023

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

patricklx commented Jul 22, 2023

Copy link
Copy Markdown
Contributor Author

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

patricklx commented Jul 22, 2023

Copy link
Copy Markdown
Contributor Author

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

@NullVoxPopuli

Copy link
Copy Markdown
Collaborator

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

@patricklx

Copy link
Copy Markdown
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.

5 participants