[VEL-1590] Road to Glint support: Stage 2 — Kickoff Glint support #351
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
phndiaye
force-pushed
the
pn/chore/glint-setup
branch
from
7f3d039
to
2cbd68b
Compare
phndiaye
commented
Dec 27, 2023
types/glint.d.ts
Outdated
Comment on lines
1
to
26
| import '@glint/environment-ember-loose'; | ||
| import '@gavant/glint-template-types/types/ember-intl/helpers/t'; | ||
| import '@gavant/glint-template-types/types/ember-truth-helpers/and'; | ||
| import '@gavant/glint-template-types/types/ember-truth-helpers/eq'; | ||
| import '@gavant/glint-template-types/types/ember-truth-helpers/not'; | ||
| import '@gavant/glint-template-types/types/ember-truth-helpers/or'; | ||
|
|
||
| import type RenderModifiersRegistry from '@ember/render-modifiers/template-registry'; | ||
| import TranslationHelper from 'ember-intl/helpers/t'; | ||
| import And from 'ember-truth-helpers/helpers/and'; | ||
| import Eq from 'ember-truth-helpers/helpers/eq'; | ||
| import Not from 'ember-truth-helpers/helpers/not'; | ||
| import Or from 'ember-truth-helpers/helpers/or'; | ||
|
|
||
| declare module '@glint/environment-ember-loose/registry' { | ||
| export default interface Registry extends RenderModifiersRegistry { | ||
| t: typeof TranslationHelper; | ||
| and: typeof And; | ||
| eq: typeof Eq; | ||
| not: typeof Not; | ||
| or: typeof Or; | ||
| } | ||
| } |
There was a problem hiding this comment.
Types setup for render-modifiers, ember-intl, and ember-truth-helpers
addon/components/o-s-s/button.hbs
Outdated
There was a problem hiding this comment.
This component is totally typesafe now, and when setup, the Glint language server should give you annotation on the arguments it needs along w/ their types :)
| style?: IconStyle; | ||
| interface OSSIconSignature { | ||
| Args: { | ||
| icon: keyof typeof IconNames | string; // TODO: See https://github.com/FortAwesome/Font-Awesome/blob/6.x/js-packages/%40fortawesome/fontawesome-common-types/index.d.ts. |
There was a problem hiding this comment.
Turns out some icons we use are not listed, which is weird because they are in the right version 🤔
addon/components/o-s-s/panel.ts
Outdated
There was a problem hiding this comment.
This is just to put a signature on a template-only component. Alternatively, we could create an empty component class and have the args defined just like other components you can see migrated in this PR.
| await click('div'); | ||
| assert.ok(this.redirectStub.calledOnceWithExactly(this.url, '_self')); | ||
| sinon.restore(); | ||
| }); | ||
|
|
||
| test('it redirects to the url on the provided target', async function (assert) { | ||
| this.redirectStub = sinon.stub(window, 'open'); | ||
| await render(hbs`<div {{on "click" (redirect-to url=this.url target="_blank")}}>link</div>`); | ||
| await render<TestContext>( |
There was a problem hiding this comment.
TestContext usage example.
upfluence-bot
added
reviewed/needs-work
reviewed/needs-review
and removed
reviewed/needs-review
reviewed/needs-work
labels
phndiaye
changed the title
phndiaye
force-pushed
the
pn/chore/glint-setup
branch
from
97a99fd
to
1593088
Compare
phndiaye
force-pushed
the
pn/chore/core-deps-update
branch
from
d918b75
to
3de1314
Compare
…lint's requirements
phndiaye
force-pushed
the
pn/chore/glint-setup
branch
from
c20a128
to
146ed68
Compare
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
Although we have pretty good Typescript support across the component library, it unfortunately can't see what's happening in templates, meaning we miss out on any typechecking and other helpful annotations it can provide.
Glint enables TypeScript to understand your templates as well, meaning you get end-to-end type safety for your entire application. You also get editor support for things like documentation and type info on hover, jumping to definitions, and automated codefixes and refactorings.
Interesting take on this too:
What does this PR do?
Built on top of the core dependencies updates, this PR introduces Glint setup and:
ember-codemod-args-to-signaturecodemod)thelper and ember-truth-helpers common helpers (via https://github.com/Gavant/glint-template-types) as well as modifier typings for ember-render-modifiers (did-insert,will-destroy, etc.)For the moment, only a couple component are considered "type-safe" and everything else is marked as type-unsafe. To enable full support, the best way (i believe) would be to first port components that depends on nothing (just HTML, no helpers, not using other components) then go up the chain with "composite" components (molecules?) and finish w/ the most complex ones.
To enable editor support for Glint, one needs to install the dedicated Glint Template Server extension:
What will affect us?
You'll notice that there are
glint-nocheckcomments in both the templates and the tests. This is what allows us to mark the components as type-unsafe so we can incrementally migrate them later.While it doesn't make the templates way uglier, it does for tests files until we make them typesafe. Eventually, most of those comments will disappear as we increase the typesafety coverage.
However, It also forces us to pass the
TestContextwhen rendering a component w/ athisusage in the args. This is something that's a good practice anyway but we were not doing it until now and it can affect the DX a bit. IMO, this can motivate us to always define a globalrenderComponentfunction in the tests so we don't have to put thatrender<TestContext>(...args)everytime.Unfortunately (or fortunately 🤷♂️ ), Glint covers the same file covered by the
tsconfigso we cannot opt-out of it ontestsdirectory.Final note: this PR only sets kicks off Glint support inside this package. Other ones in apps will follow to see the types are used.
Good PR checklist