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

Replacing moduleName in template meta #940

Open
ef4 opened this issue Jul 27, 2023 · 11 comments
Open

Replacing moduleName in template meta #940

ef4 opened this issue Jul 27, 2023 · 11 comments

Comments

@ef4
Copy link
Contributor

ef4 commented Jul 27, 2023

Historically, Ember compiles a moduleName into the wire format of each template. The Ember Inspector displays this when navigating the hierarchy of components.

This isn't really viable going forward, for two reasons:

  1. moduleName as traditionally understood doesn't exist reliably anymore

    • it used to be the "runtime" name of a module, which was unambiguous because all packages were forced into one flat namespace. But we don't do that anymore. Under embroider (and even in classic builds when consuming v2 addons), we follow normal node_modules resolution, so packages are not a flat namespace.
    • the mapping from publicly-visible name to internal-name can be complicated, based on the full feature set of package.json exports. For us to try to produce a traditional moduleName we would need to faithfully run that transformation, backwards.
  2. The existence of Template Tag means that templates are not one-to-one with modules anymore. So identifying which module a component lives in is not enough to unambiguously locate it.

Since templates are javascript values (or to be precise: templates are a detail of components and components are javascript values), it's worth asking: why do we think we need to label them with their module names, when we don't do that with any other javascript values?

I would like to suggest that instead of trying to label components by their template moduleName, we can change the inspector to offer a "jump to definition" button on components.

  • For class-based components, window.inspect(theComponentInstance.constructor) already works.
  • But for template-only components, there's no function present to point at.
    • we could introduce one inside the wire format of the template
    • or we could make templateOnlyComponent() capture an Exception and save its stack so that it would be possible to navigate back to the site at which the component was defined.

A related concern here is that AST transforms can see moduleName and I know some rely on it. They need a deprecation path to stop doing that. It's going to troll them anyway as soon as people are using template tag.

@patricklx
Copy link

glimmer vm is also probably passing it as the only template information. captureRenderTree items has a property ``template` which is the same as moduleName

@chancancode
Copy link
Member

Isn’t the scope closure good enough?

@chancancode
Copy link
Member

Sorry I should be more specific, I was responding to the need for a function to “point at” for jumping to the definition; scopeFn could serve that purpose rather than introducing one specifically for debug purposes only, right?

@lifeart
Copy link

lifeart commented Aug 4, 2023

Copying my comment from emberjs/ember-inspector#2425 (comment) here:

Wee may still need moduleName to support hot module replacement for components (in vite), at the moment we map changed component files to rendered components by moduleName resolved from getComponentTemplate or component itself. (https://github.com/lifeart/demo-ember-vite/blob/master/plugins/hot-reload.ts#L169)

image

@chancancode
Copy link
Member

chancancode commented Oct 24, 2023

Today in the tooling meeting we discussed this and are generally in favor of trying to eliminate moduleName from <template> tag by default.

I looked around it seems like most places already assume moduleName could be undefined and generally have fallbacks like ?? (unknown module)

I propose that

  1. we add a { moduleName = undefined } option to template()
  2. for <template>, depending on how we feel about using the "attributes space", either
    <template moduleName="...">...</template>
    
    or
    // moduleName ...
    <template>...</template>
    
    (to be consistent with Whitespace handling for <template> tag #982)
  3. if it seems like it is important to have this for dev mode and/or backwards compatibility, we ship a babel plugin that adds back the above in a best-effort compatible way

@chancancode
Copy link
Member

I don't think HMR should be using moduleName at all, it should just replace the imported value.

import Foo from "./foo";
import bar from "./bar";

<template>
  <Foo @foo="..." />
  {{bar "some" "args"}}
</template>

Should be transformed into something like

import { tracked } from "@glimmer/tracking";
import Foo from "./foo";
import bar from "./bar";

// TODO eliminate this when not import.meta.hot
const __imports__ = new (class {
  @tracked Foo = Foo;
  @tracked bar = bar;
})();

if (import.meta.hot) {
  import.meta.hot.accept('./foo', (Foo) => __imports__.Foo = Foo);
  import.meta.hot.accept('./bar', (bar) => __imports__.bar = bar);
}

<template>
  <__imports__.Foo @foo="..." />
  {{__imports__.bar "some" "args"}}
</template>

Then everything should just work.

@NullVoxPopuli
Copy link
Contributor

what about just "name"?

export const Foo = <template>

</template>;

gets transpiled to:

// ...
export const Foo = template('...', { name: 'Foo', /* ... */ });

no user-land customization?

@chancancode
Copy link
Member

The point of the feature is for backwards compatibility anyway and I think we either try to preserve existing behavior as much as possible or not do it at all

@chancancode
Copy link
Member

if you want it name-ed for some reason, just use a class:

class Foo { <template>...</template> }

That would naturally give you the .name property and you can use the normal pragma to tell the minifier to leave it alone (or not)

@NullVoxPopuli
Copy link
Contributor

it's just <template ...attribute space> would require more syntax work for some editors / tools, so my preference would be to only add something to attribute space if it's going to be a future-facing feature, rather than a backcompat one

@chancancode
Copy link
Member

chancancode commented Oct 24, 2023

I think that space is spec-ed/reserved for content-tag, so it may need to happen anyway (e.g. depending on #982), but I agree I wouldn't do it just for this. So, IMO if we are using it for other things, then we should use it for this, otherwise the anything is fine.

That being said it does have its issues: e.g. what if you want to make it conditional based on import.meta.DEBUG. However, content-tag probably has to support that in general anyway.

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

No branches or pull requests

5 participants