Skip to content

Conversation

asakusuma
Copy link

@asakusuma asakusuma commented Jan 18, 2023

While testing out ember-cli/ember-cli-htmlbars#762, I realized that we probably don't want the precompiled template moduleName to be an absolute path, as that would be exposing production box internals. It also doesn't match the previous behavior of older templates where moduleName is relative.

This change constructs the moduleName using the path.relative of the babel cwd and filename options, instead of just filename. I tested this change out locally in an ember app. Instead of using cwd and filename with path.relative, we could also use state.file.opts.sourceFileName, which works here when I run locally, but not sure it would work universally. In theory, we could use filenameRelative, but anecdotally that option is not available when I build my own app locally, so it may be a new option.

Without the source code change, verified tests fail:

Screenshot 2023-01-17 at 5 07 45 PM

@asakusuma
Copy link
Author

@ef4 friendly reminder to take a look at this PR when you get a chance

@asakusuma
Copy link
Author

@ef4 bump 😃

@ef4
Copy link
Contributor

ef4 commented Feb 13, 2023

Sorry, I wrote up a reply last time you pinged me but somehow it never submitted.

First: I think it's dubious that we keep moduleName in production builds at all. It exists for debugging. I would be interested in dropping it entirely. But since we still have it...

It also doesn't match the previous behavior of older templates where moduleName is relative.

I think the older behavior was not relative, it was a different, third thing: it was the notional "runtime" module name, which is neither a true absolute filename nor a relative filename. The problem is, in general that thing is hard to reverse engineer from a true on disk filename. However...

This code can't actually assume that the filename from babel actually is a real on-disk file, because under classic builds it is not. In that case it's already the aforementioned notional runtime name. I think you will get very weird results from passing that through path.relative like this.

So I don't think this PR is safe as is. Possible paths forward:

  • if your main concern is the information hiding, can you experiment with dropping moduleName entirely from production mode builds and see if that actually works in your apps?
  • if you really want to keep moduleName for backward-compatibility, then we should try to implement the old behavior.

Finally, the core implementation in plugin.js is supposed to not be node-specific. It works in browser environments too, for example. It shouldn't import from path. So if we're going to do node-specific path mangling it would need to go in node-main.ts instead.

@ef4
Copy link
Contributor

ef4 commented Feb 13, 2023

Addendum: it's really the ember template compiler that chooses to emit moduleName in the actual output code. We set it both for that purpose and so that AST plugins can see it. It's entirely possible that we're failing to pass some other option to the ember-template-compiler that would stop it from being emitted in prod builds.

@asakusuma
Copy link
Author

No worries, thanks for the explanation. So we actually do need the old behavior, whatever that may be, for a legacy internal addon that we are in the process of replacing. I agree that long term it would be good to get rid of moduleName in production builds.

I'll do some spelunking to try and reverse engineer the old behavior, I'm all ears if you have any insight on the correct way to do this.

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.

2 participants