Fix importing of template-only components in V2 addons

[NullVoxPopuli]

Accompanying repo for testing: https://github.com/NullVoxPopuli/ember-addon-v2-template-only-import

TODO

• importing template-only component
• template-only component included in app re-exeports

Resolves: #1121

[ef4]

Think of the resolveId as providing an extra fallback.

In pseudocode, you need to do something like this where resolveId handles relative paths:

resolveId(id) {
  // ...

  // for relative paths inside our own package, first try normal resolving.
  // This is calling https://www.rollupjs.org/guide/en/#thisresolve
  let result = this.resolve(source);
  if (result) {
    return result;
  }

  // only if that doesn't work, try to rewrite to .hbs
  result = this.resolve(source.replace(/\.js$/, '.hbs'));
  if (result) {
    return result;
  }

[NullVoxPopuli]

imported hbs files are now inlined -- which, imo, is a great thing to do, but it's not a safe thing to do as these components previously were still available to the globals resolver.

adding hbs to publicEntrypoints' regex does the trick, but it also un-inlines the co-located templates for components that also have an associated class. 🤔
So, it seems publicEntrypoints needs to "emitFile" for template-only components? (and we don't add hbs to publicEntrypoints' regex)

[ef4]

Co-located templates (meaning app/components/*.hbs) are never available to the resolver. Compare with a classic app where the behavior is the same.

They always become an internal detail of the component JS module.

[NullVoxPopuli]

Co-located templates (meaning app/components/*.hbs) are never available to the resolver.

I don't understand,

• if app/components/button.hbs exists,
• then I can do <Button /> anywhere in the app (available to the resolver)

Or do you mean that, at the time the resolver becomes aware of anything, it's already been converted to JS?
So a standalone template-only button.hbs is converted button.js so button.hbs is only a "syntactic sugar" that's supposed to be compiled away fairly quickly.

🤔 maybe the approach is wrong here -- maybe we need addon.hbs() to instead convert hbs template-only to js and merge hbs+js co-located components.

What I don't understand (at the moment): is the current behavior of the hbs plugin:

      let input = readFileSync(id, 'utf8');
     let code =
       `import { hbs } from 'ember-cli-htmlbars';\n` +
       `export default hbs${backtick}${input}${backtick};`;
     return {
       code,
       id: id + '.js',
     };

would this not collide with the class half of a component?
This looks like it's doing what I want, but it seems to be a sneaky way to fallback to rollup inlining template files some how? idk

[ef4]

Or do you mean that, at the time the resolver becomes aware of anything, it's already been converted to JS?

Yes, that. If you have:

app/components/thing.js
app/templates/components/thing.hbs

Those are two separate entries in the runtime resolver that ember puts together after finding both at runtime. But if you have

app/components/thing.js
app/components/thing.hbs

Those become a single module from the perspective of the runtime resolver. And if you have

app/components/thing.hbs

It also becomes a single JS modules from ember's perspective.

imported hbs files are now inlined -- which, imo, is a great thing to do, but it's not a safe thing to do as these components previously were still available to the globals resolver.

This is misunderstanding how things work. We already let rollup inline lots of things, including templates. That has nothing to do with what will be accessible to the resolver. That's controlled entirely by the appReexports, which in turn depend on the publicEntrypoints. publicEntrypoints is how you're telling rollup which things can't be inlined because they need to be addressable from the outside.

[NullVoxPopuli]

app/components/thing.js
app/templates/components/thing.hbs

I forgot this option existed 😅

It also becomes a single JS modules from ember's perspective.

or (with v2 addons), will soon be (what I'm trying to figure out with this PR as well)

hat's controlled entirely by the appReexports, which in turn depend on the publicEntrypoints.

but it's all files based, yeah? like, you can't say "inline everything into one file, and then generate app re-exports for that", like,

// dist/_app_/components/foo.js
export { Foo as default } from 'my-addon';

// dist/_app_/components/bar.js
export { Bar as default } from 'my-addon';

[NullVoxPopuli]

Speaking of, I think I just did it 🥳

https://github.com/NullVoxPopuli/ember-addon-v2-template-only-import
now has:

• template-only component converted to JS
• template-only component imported by co-located component
• template-only component now in app re-exports

[ef4]

Where does this emit the call to templateOnlyComponent()? That is critical to generate correct template-only components.

The idea is that if somebody tries to load a nonexistent JS file beside an HBS file, we fill in the contents of the JS file with something like:

import templateOnlyComponent from "@ember/component/template-only";
export default templateOnlyComponent();

That is probably sufficient as long as the colocation plugin also has a chance to run and handle that. Otherwise we would also need to synthesize the import of the template and the setComponentTemplate here.

[ef4]

The linked demo app seems to misunderstand the intended semantics of hbs imports in the v2 addon spec. This:

https://github.com/NullVoxPopuli/ember-addon-v2-template-only-import/blob/2f31e515f66437702b58b287c2e3184aa9495ff6/my-addon/src/components/demo/index.js#L5

should not produce a component, it should be just the template. We need this both for compatibility and because we use it internally to bind together a component's JS and HBS. That is, if you author both foo.js and foo.hbs, the colocation inserts an import TEMPLATE from './foo.hbs'; setComponentTemplate(TEMPLATE, whatever); into foo.js. So an import ending in an explicit .hbs extension needs to mean "just the template", not a component with that template.

[ef4]

I will add that having the demo repo to go with this is a very good idea and once we get it to demonstrates all the features we want it can move directly into this repo as a test suite.

[NullVoxPopuli]

@ef4 I think I did it, can you verify?

also, it'd be really handy to have automated tests in here -- have any thoughts on how that should be set up?

maybe in the scenarios folder or something?

[lifeart]

There is a possible ways to test resolveId - https://github.com/rollup/plugins/blob/master/packages/virtual/test/test.js#L7
We could use virtual FS for it, and plugin could accept fs implementation in constructor

[NullVoxPopuli]

There is a possible ways to test resolveId

I was thinking testing a full build would be useful because the hbs plugin needs resolveId and resolve to work in concert with one another

[NullVoxPopuli]

This is rebased, and my little demo repo works.

example output:

❯ cat dist/components/demo/index.js 
import { _ as _applyDecoratedDescriptor, a as _defineProperty, b as _initializerDefineProperty } from '../../_rollupPluginBabelHelpers-2eca8644.js';
import { setComponentTemplate } from '@ember/component';
import { hbs } from 'ember-cli-htmlbars';
import Component from '@glimmer/component';
import { tracked } from '@glimmer/tracking';
import templateOnly from '@ember/component/template-only';
import Out from './out.js';

var TEMPLATE = hbs("Hello there!\n\n<this.Out>{{this.active}}</this.Out>\n\n<this.Button @onClick={{this.flip}} />\n<this.Button2 @onClick={{this.flip}} />\n");

var BlahButton = setComponentTemplate(hbs`<button {{on 'click' @onClick}}>
  flip
</button>

`, templateOnly());

var _class, _descriptor;
let Demo = (_class = class Demo extends Component {
  constructor(...args) {
    super(...args);

    _defineProperty(this, "Button", BlahButton);

    _defineProperty(this, "Button2", BlahButton);

    _defineProperty(this, "Out", Out);

    _initializerDefineProperty(this, "active", _descriptor, this);

    _defineProperty(this, "flip", () => this.active = !this.active);
  }

}, (_descriptor = _applyDecoratedDescriptor(_class.prototype, "active", [tracked], {
  configurable: true,
  enumerable: true,
  writable: true,
  initializer: function () {
    return false;
  }
})), _class);
setComponentTemplate(TEMPLATE, Demo);

export { Demo as default };

The important thing I was trying to make sure happened:

• imports of the same template-only module are de-duped
• template-only modules acquire setComponentTemplate + templateOnly()

So, success :D

[NullVoxPopuli]

I've left the execa usage, because:

• I don't know how to
  • get an addon project from a PreparedApp (so that I can call execute)

I don't think this sort of problem should be a blocker, because seems like we need to add behaviors to scenario-tester

• add some way of getting a linked project from a PreparedApp
  (only APIs right now are dir and execute)

Also, this PR has been open for a very long time (mostly my fault!) and the community needs the capabilities <3

Next up, I'll be working on seeing if this is still an issue: #1134

[ef4]

This is not correct. An import with an explicit hbs extension should not give you back a component. It gives back only the template. Which would not be invokable the way you're using this.

This is the same thing I pointed out in #1126 (comment)

[ef4]

This is assuming that source will always be a relative path, but that's not always true. For example, if somebody tries to import from an NPM package name that isn't installed, they will end up here, and we don't want to be appending package names to the importer path.

This really should be another call to this.resolve()

[ef4]

This comment is misleading. The globals resolver isn't really relevant to this. You control global resolving by deciding whether or not to put a module into appReexports. But this code runs for your modules whether or not they're in appReexports.

[ef4]

Similar to the meaning in imports, we need a way to talk about template files, so people need to be allowed to put explicit .hbs here like this. But that can't mean the same thing as the template-only component. The template-only component is a JS file -- even if that JS file is synthesized by the build, rather than typed by the author.

[ef4]

@NullVoxPopuli thanks for pushing this forward, and especially for getting good tests in place. I was able to sort out the implementation.

[NullVoxPopuli]

Ah, this makes sense, now that i see it 🤦

[NullVoxPopuli]

This is way simpler than what i was doing 🤔

[NullVoxPopuli]

Should we throw on non-js extensions in here and is app-re-exports?

[NullVoxPopuli]

Ah,. This was probably what i was missing (and maybe all the changes in this file).

Because i was so focused on the other plugin, i didn't remember how in interacts with this plugin.