[Glimmer2] Ensure attributeBindings are not allowed

[asakusuma]

No description provided.

[chadhietala]

Not sure if this is supposed to be a runtime error or a compile error. It seems like there is an AST transform that would assert this.

https://github.com/emberjs/ember.js/blob/master/packages/ember-template-compiler/lib/plugins/transform-old-binding-syntax.js#L24

/cc @krisselden

[krisselden]

Build time error that the plugin provides is a better end user experience we should make this pass by enabling the transform on the glimmer templates rather than a runtime thing /cc @rwjblue

[rwjblue]

Yes, 100% agree. Build time error gives much more info to the user (module + line + column).

[asakusuma]

Makes sense, taking another stab at this now

[asakusuma]

Do I need to be more defensive about ensuring that [plugins.ast] always has the OldSyntax plugin? If someone called compile and has their own set of plugins, they wouldn't get the OldSyntax plugin that is doing the check.

Edit: looks like the existing code is being defensive about this. I'll update

[rwjblue]

I'd rather put this in packages/ember-template-compiler instead of nesting it within the ember-glimmer package. I'm sure there is a reason for doing it here for now, can you recall what it is?

[asakusuma]

From looking at the directory structure, I had just assumed that ember-glimmer/ember-template-compiler was the right place for anything in packages/ember-template-compiler that was getting ported over to glimmer2. That was the only rationale.

Pretty sure we could easily use the existing compileOptions instead of the new, inline one I added in packages/ember-glimmer/lib/ember-template-compiler/system/compile.js if that is preferable.

[chadhietala]

I think there needs to be some clarification here. If you just flip the test from @htmlbars to @test I believe the test will pass. This is due to the fact that this assertion already exists and the AST transforms were turned on in #13251, however we have an issue. The issue is that we run the tests with enableOptionalFeatures on, this means that the AST transforms are being tested only for the ember-glimmer package and not the ember-htmlbars package. While this is "ok" as the AST is the same between the two templating engines, we should continue testing both until we flip the ember-glimmer flag to be the default engine.

@krisselden has been working to get to a point where we can abstract and re-export htmlbars and glimmer through an ember-templates package. This will allow us to ensure we can toggle between to the two engines for these types of tests and acceptance tests.

@asakusuma can you confirm or deny that this test Just Works™ by flipping it to @test with no other changes?

[asakusuma]

It does not just work when I flip it to @test

[asakusuma]

@chadhietala see my note reply to @rwjblue's comment, but if the AST is the exact same and we can assume the same transforms, I can just use the existing compileOptions instead.

The reason the tests don't "Just work" is because you have to pass in the transforms. See work being done in packages/ember-glimmer/lib/ember-template-compiler/system/compile.js.

EDIT: I think we can just pull in the plugins export from packages/ember-template-compiler/lib/plugins instead of setting up a parallel plugins set for glimmer2

[rwjblue]

Yes, I'd prefer to just use the top level packages/ember-template-compiler. Anything in packages/ember-glimmer/lib/ember-template-compiler will have to be moved eventually anyways, so we might as well just start there and save ourselves a little extra work in the long run...

[homu]

☔ The latest upstream changes (presumably #13345) made this pull request unmergeable. Please resolve the merge conflicts.

[asakusuma]

@rwjblue fixed. I tried applying all the htmlbars plugins, but that caused a bunch of test failures, so for now I've only added the plugin needed for this specific issue.