emberjs / babel-plugin-ember-template-compilation

Babel implementation of Ember's low-level template-compilation API
9 stars 11 forks source link

Use the relative path for precompiled moduleName #13

Closed asakusuma closed 1 year ago

asakusuma commented 1 year ago

While testing out https://github.com/ember-cli/ember-cli-htmlbars/pull/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 commented 1 year ago

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

asakusuma commented 1 year ago

@ef4 bump 😃

ef4 commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

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.