emberjs / babel-plugin-ember-template-compilation

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

2.2.2 breaking change: globals are now 'not in scope' for templates? #38

Open davidtaylorhq opened 6 months ago

davidtaylorhq commented 6 months ago

There appears to be a breaking change between 2.2.1 and 2.2.2. Previously, in gjs files, we could reference JS globals directly as helpers. For example, we were using @something=(Boolean this.blah) to cast a value to a boolean.

Now, we get the error:

Error: Attempted to resolve a helper in a strict mode template, but that value was not in scope: Boolean

Adding const Boolean = window.Boolean to the file works around the problem.

This problem is not limited to helpers. Something like {{document.title}} previously worked, but now throws a 'not in scope' error.

ef4 commented 6 months ago

LIke https://github.com/emberjs/babel-plugin-ember-template-compilation/issues/38, this was intentional because in general there's no way to statically disambiguate between javascript globals and future ember keywords. That is the tradeoff of not making people import keywords.

We do support globalThis, so I would expect @something=(globalThis.Boolean this.blah) to work.

We can decide to support a standard list of platform-provided globals, including Boolean and document. Perhaps we should do that right away to mitigate the impact of this change.

The thing we don't want to do is support arbitrary globals. Like, if somebody decides they don't like importing their component all the time and tries window.CustomButton = CustomButton, that doesn't make <CustomButton /> work in all components.

chancancode commented 6 months ago

Another (IMO more important) consideration is html tags, or rather, for the scoping rule to work consistently across all positions (<___> vs {{___}}); say we support window and document, if HTML later introduces <window> or <document> elements, there will be no way to invoke them (without making convoluted excepts to the scoping rules)

chancancode commented 6 months ago

Not to say that I am against supporting window or document for that reason, but we should keep that in mind every time we consider adding to that list

NullVoxPopuli commented 6 months ago

I would like common casts added to the list: Boolean, String (helpful for glint)

And utility namespaces: Math, JSON, Date, URL

And Common globals: window, document, navigator, localStorage

Tho, given the congern ef4 raises, i can deal with destructuring globalThis, as awkward as it is. Tho, elements must all be lowercase, so the namespaces and casts seem safe

Techn1x commented 1 month ago

this was intentional

breaking but marked as a patch?

Was wanting to consume this 2.2.2 patch to fix https://github.com/ember-cli/ember-template-imports/issues/229 but as Simon mentions it's got other regressions like this.


Likely related, I'm also having issues with enums that _are_ named and imported (click to expand) ```ts // app/components/alert.gts export enum AlertType { Information = 'information', Warning = 'warning', Critical = 'critical', Success = 'success', NewFeature = 'new-feature', } export component Alert: TOC = ``` results in error at runtime ``` Uncaught (in promise) Error: Attempted to resolve a value in a strict mode template, but that value was not in scope: AlertType ```

EDIT: opened issue for enums https://github.com/emberjs/babel-plugin-ember-template-compilation/issues/65

ef4 commented 1 month ago

Globals were not supposed to work, it was a bug that they worked, the bug is dangerous because if people come to rely on it, Ember is prevented from ever adding a new keyword again, because the template compiler has no way of distinguishing it from an in-scope global.

Enums may be a different bug, please file a separate issue for that one.

Techn1x commented 1 month ago

Enums may be a different bug, please file a separate issue for that one.

Thanks, I've put the bug example behind an expand block for the time being (and simplified the example as well)

Techn1x commented 1 month ago

Globals were not supposed to work

That's news to me, and IMO likely not known by anyone outside of the core devs 😅

I think anyone using any number of GTS files are likely to be using globals all over the place. Especially so given typescript is totally happy with it.

the bug is dangerous because if people come to rely on it, Ember is prevented from ever adding a new keyword again, because the template compiler has no way of distinguishing it from an in-scope global.

I agree, though it being a patch (and without mentioning it in the changelog at all......) might have been misguided. But having now looked over other GH issues on the repo regarding 2.2.2, I suspect that has already been made clear by the number of issues reported, and I shouldn't have brought it up.

Techn1x commented 1 month ago

I would like common casts added to the list: Boolean, String (helpful for glint)

Array would be a good one for that list

I just saw a colleague use @chartMargin={{Array 10 20 10 30}} and it blew my mind. And here I have been importing & using array ember helper like a chump, when the native constructor is clearly better 😄

NullVoxPopuli commented 1 month ago

Woah, yes