chore: Fix main branch typescript errors #83

ynotdraw · 2023-05-17T14:43:30Z

Description

Get the build for this repo green again so that we can put up a PR to export the Glint Template Registry. Once we have the registry, we can use this in toucan-core to create Popover, Select, and Multiselect properly.

Here's everything this PR covers:

Drop TS support less than 4.8. Glint only supports TS 4.8+
Upgrade Glint dependencies to latest v1
Fix TS errors around "owner"/"Resolver"
Add @ember/string as a dependency
Resolve TS errors in tests. Likely need to extend the TestContext due to using this.setProperties
Drop commitlint
- Our other open source repos dropped this and moved to change sets instead
- I am happy to move this over to changesets for consistency in a follow up PR as I feel like all of our open source repos we work in regularly should follow similar conventions for DX
- See this comment

⚠️ NOTE: ⚠️ We'll want to do a breaking change release due to dropping TS support and moving to Glint in another downstream PR. I'm hoping to get this done over the next few days 🤞.

Started getting type errors with resolver: "Module '"@ember/owner"' has no exported member 'Resolver'. Did you mean to use 'import Resolver from "@ember/owner"' instead?" ember-resolver now ships with their own types rather than the types namespace

ynotdraw · 2023-05-19T13:22:25Z

test-app/package.json

@@ -63,6 +62,7 @@
    "@types/ember__engine": "^4.0.4",
    "@types/ember__error": "^4.0.2",
    "@types/ember__object": "^4.0.5",
+    "@types/ember__owner": "^4.0.3",


We need this, otherwise when running glint we get the following:

../node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@types/ember__application/index.d.ts:17:17 - error TS2614: Module '"@ember/owner"' has no exported member 'Resolver'. Did you mean to use 'import Resolver from "@ember/owner"' instead? 17 import Owner, { Resolver } from '@ember/owner'; ~~~~~~~~ ../node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@types/ember__debug/container-debug-adapter.d.ts:1:10 - error TS2614: Module '"@ember/owner"' has no exported member 'Resolver'. Did you mean to use 'import Resolver from "@ember/owner"' instead? 1 import { Resolver } from '@ember/owner'; ~~~~~~~~ ../node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@types/ember__engine/index.d.ts:15:10 - error TS2614: Module '"@ember/owner"' has no exported member 'Resolver'. Did you mean to use 'import Resolver from "@ember/owner"' instead? 15 import { Resolver } from '@ember/owner'; ~~~~~~~~ ../node_modules/.pnpm/[email protected]_j5milmbhnvuuqbh2i3lfyo7nii/node_modules/ember-qunit/types/index.d.ts:10:10 - error TS2614: Module '"@ember/owner"' has no exported member 'Resolver'. Did you mean to use 'import Resolver from "@ember/owner"' instead? 10 import { Resolver } from '@ember/owner'; ~~~~~~~~ ../node_modules/.pnpm/[email protected][email protected]/node_modules/ember-resolver/index.d.ts:1:10 - error TS2614: Module '"@ember/owner"' has no exported member 'Resolver'. Did you mean to use 'import Resolver from "@ember/owner"' instead? 1 import { Resolver as ResolverContract } from "@ember/owner";

There is also the option to use the built in types for easier addon dev!

I was thinking of giving those a go! It'd be nice to remove all of the @types/ember__ packages.

While I have you here - do you happen to know why glint seemingly type checks within node_modules like this? I'm used to Next.js projects where the tsconfig uses skipLibCheck: true and we don't get these types of issues. As a test, I added skipLibCheck in the tsconfig and I no longer got this error, but then realized I needed to add this dependency.

glint seemingly type checks within node_modules like this?

TS (and Glint) must derive the types you use somehow, they can't just come from nowhere, ya know?

I'm used to Next.js projects where the tsconfig uses skipLibCheck: true

as a library author / maintainer, one may not use skipLibCheck safely because it propagates hidden errors to consumers.
The library one authors must be as correct as reasonable, and skipLibCheck accidentally avoids that correctness.
It's only reasonable to use in apps / or maybe aka "endpoint consumers".

Additionally, to be the best stewards we can be, every library author probably wants to have these settings, if using pnpm:
https://github.com/emberjs/rfcs/pull/907/files#diff-116076a2d4078dfbf8de672a7b681ed1635715f3acfbfd31a1e7009251eef70dR123

# all peer dependencies must be declared or forwarded to the consumer auto-install-peers=false # we want true isolation in addons -- if a dependency is not declared, we want an error reslove-peers-from-workspace-root=false

but then realized I needed to add this dependency.

you'll likely have a lot less issues with the built in types.
The way DT manages types by allowing all chaos via * dep ranges is madness. They are doing their best though. but it's madness.

Here is an example PR I did recently for the v2 addon bluerpint to adopt the built in types.
embroider-build/addon-blueprint#118

Errors / @types/etc

Regarding your errors though, you may have the quickest (and smallest diff) luck by

upgrading all type deps to latest

deleting the lockfile

re-install (re-generating the lockfile)

remove @types for packages which now ship their own types (ember-resolver, @ember/string, @ember/test-helpers, etc)

Oh, this is super helpful! Thank you! 🙇

reslove-peers-from-workspace-root=false

Just checking that you didn't copy paste that from somewhere? I guess it's probably meant to be resolve although everyone could do with some res love!

lol. yeah, it's a typo. resolve is correct 🙈

ynotdraw · 2023-05-19T13:50:18Z

.github/workflows/ci.yml

-          - [email protected]
-          - [email protected]
+          - [email protected]
+          - [email protected]


Glint requires typescript >= 4.8. We had a similar PR here

ynotdraw · 2023-05-22T13:06:07Z

test-app/tests/integration/components/velcro-test.ts

@@ -137,6 +137,8 @@ module('Integration | Component | velcro', function (hooks) {
            <div {{velcro.hook}}>velcroReference</div>
            <div id="velcro1" {{velcro.loop}}>Velcro</div>
          </Velcro>
+          {{!-- Having an issue referencing a typed 'this' in the modifier --}}
+          {{! @glint-ignore }}


For some reason I couldn't get this context to be my extended TestContext following something like: https://github.com/typed-ember/ember-cli-typescript/blob/master/docs/ember/testing.md#addon-tests

test('can pass in distance', async function (this: TestContext & { offsetDistance: number; }, assert) { let offsetDistance = 10; this.set('offsetDistance', offsetDistance); // This doesn't work either // this.offsetDistance = offsetDistance; await render(hbs` <div style="display: flex"> <Velcro @placement="bottom-start" as |velcro|> <div {{velcro.hook}}>velcroReference</div> <div id="velcro1" {{velcro.loop}}>Velcro</div> </Velcro> {{!-- "this" here is void! Strange! --}} <Velcro @offsetOptions={{this.offsetDistance}} @placement="bottom-start" as |velcro|> <div {{velcro.hook}}>velcroReference</div> <div id="velcro2" {{velcro.loop}}>Velcro</div> </Velcro> </div> `); let velcro1 = findElement('#velcro1'); let velcro2 = findElement('#velcro2'); assert.strictEqual( velcro1.getBoundingClientRect().top + offsetDistance, velcro2.getBoundingClientRect().top ); });

Whenever this is referenced in the template, it says the type is void and points to @glint/template's TemplateContext rather than the overriding test context. Any pointers here would be wonderful!

imo, best to use gts

Will do! Thanks as always, @NullVoxPopuli ! 🙇

ynotdraw · 2023-05-22T18:07:20Z

commitlint.config.cjs

Went ahead and deleted this as our other repos are no longer including this due to the move to changesets where we can control releases and commits can be squashed with helpful messages anyway. Getting errors in CI about commits is really tough, especially when the error messages aren't helpful at all.

We don't want to create a release just yet using semantic-release anyway, as there's still more to do to get this completely functional with toucan-core + Glint in general for external consumers. For example, we'll need to add an exported template-registry. I plan on doing this right after this PR merges, as I want to keep PRs as small as possible so they're easier to follow.

ynotdraw · 2023-05-22T18:08:50Z

test-app/tests/integration/components/velcro-test.gts

-              {{object-keys velcro.data}}
+              {{#each-in velcro.data as |key|}}
+                {{key}}
+              {{/each-in}}


Glint was having issues with recognizing object-keys, so I reached for #each-in instead

It also isn't available as a global as globals don't exist in gts

Ah, so that's why Glint wasn't recognizing it. Makes sense! 🎉

simonihmig · 2023-05-23T08:55:18Z

ember-velcro/package.json

-    "@glint/environment-ember-loose": "^0.9.4",
-    "@glint/template": "^0.9.5"
+    "@glint/environment-ember-loose": "^1.0.2",
+    "@glint/template": "^1.0.2"


I know this PR didn't change this, but I wonder why we have these as peer deps? Other similar addons with Glint support don't have this, same for the addon blueprint. At least they should be optional, as non-TS shouldn't be force to install them. But I think it should be enough to have them as devDeps!?

Thank you! I agree here, nice call out. This should be resolved now with the latest commit

simonihmig · 2023-05-23T08:56:17Z

test-app/package.json

    "@nullvoxpopuli/eslint-configs": "^2.2.58",
    "@tsconfig/ember": "^1.1.0",
    "@types/ember": "^4.0.3",
-    "@types/ember-qunit": "^5.0.2",
-    "@types/ember-resolver": "^5.0.13",
+    "@types/ember-qunit": "^6.1.1",


I think you don't need these with the latest ember-qunit, which ships its own types. Same for @types/ember__test-helpers below.

Thank you! This should be resolved now with the latest commit

simonihmig · 2023-05-23T08:58:42Z

test-app/tests/integration/components/velcro-test.gts


      assert
        .dom('#hook')
        .hasText(
-          'x,y,initialPlacement,placement,strategy,middlewareData,rects,platform,elements',
+          'x y initialPlacement placement strategy middlewareData rects platform elements',


I don't really know what this means, but is this change expected?

Yeah! Up above we are essentially writing out the object keys to the DOM and asserting what's returned there with:

{{#each-in velcro.data as |key|}} {{key}} {{/each-in}}

Previously we were using object-keys which would comma-separate them, but Glint wouldn't recognize that, so I switched to each-in which is why it went from comma-separated to space-separated.

Alternatively, we could do something more verbose and explicit though. Maybe something like:

test('it yields the MiddlewareArguments', async function (assert) { await render(<template> <Velcro as |velcro|> <div id="hook" {{velcro.hook}}> <ul> {{#each-in velcro.data as |key|}} <li data-velcro="{{key}}">{{key}}</li> {{/each-in}} </ul> </div> <div id="loop" {{velcro.loop}}>VelcroElement</div> </Velcro> </template>); assert .dom('li') .exists( { count: 9 }, 'has the correct velcro.data object keys' ); assert.dom('[data-velcro="x"]').exists(); assert.dom('[data-velcro="y"]').exists(); // etc. });

Any preference?

Oh ok, got it. All good!

simonihmig · 2023-05-23T08:59:32Z

test-app/tests/integration/components/velcro-test.gts

@@ -127,22 +132,20 @@ module('Integration | Component | velcro', function (hooks) {
    test('can pass in distance', async function (assert) {
      let offsetDistance = 10;

-      this.set('offsetDistance', offsetDistance);


no more stupid set, yay! 😍

Co-authored-by: Simon Ihmig <[email protected]>

camskene

Nice one!

BREAKING CHANGE: Removing types and updating TypeScript version to support Glint properly. These libraries now ship with their own types, so we no longer need the ones coming from the types namespace anymore. Co-authored-by: Simon Ihmig <[email protected]>

changeset-bot · 2023-05-25T14:28:52Z

⚠️ No Changeset found

Latest commit: 370cb3b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

# [2.0.0](v1.1.0...v2.0.0) (2023-05-25) * breaking-change: Fix main branch typescript errors (#83) ([a732f56](a732f56)), closes [#83](#83) ### BREAKING CHANGES * Removing types and updating TypeScript version to support Glint properly. The main branch build is currently red due to the Glint dependency updates. This gets things back on track. We are making a breaking change as updating underlying TypeScript changes is considered as such (CrowdStrike/ember-headless-table#176). * chore: Upgrade glint dependencies * chore: Allow any ember-source + typescript * chore: Glint only supports TSv4.8+ * chore: Upgrade ember-source + resolver Started getting type errors with resolver: "Module '"@ember/owner"' has no exported member 'Resolver'. Did you mean to use 'import Resolver from "@ember/owner"' instead?" ember-resolver now ships with their own types rather than the types namespace * chore: Remove commitlint * chore: Update babel/core + qunit deps * chore: Add @types/ember__owner dependency * chore: Add ember/string as a dependency for tests * chore: Ignore glint errors for now * chore: Convert tests to gts * fix: Remove unneeded peerDependencies * fix: Remove unneeded types/ember packages These libraries now ship with their own types, so we no longer need the ones coming from the types namespace anymore.

github-actions · 2023-05-25T14:54:44Z

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

ynotdraw added 3 commits May 17, 2023 10:42

chore: Upgrade glint dependencies

6df8e5a

chore: Allow any ember-source + typescript

ca5e123

chore: Glint only supports TSv4.8+

005290f

ynotdraw force-pushed the fix-main branch from 05edef8 to 005290f Compare May 17, 2023 15:01

ynotdraw added 4 commits May 17, 2023 11:13

chore: Remove commitlint

3690063

chore: Update babel/core + qunit deps

28761d7

chore: Add @types/ember__owner dependency

2b6ed5b

ynotdraw commented May 19, 2023

View reviewed changes

ynotdraw self-assigned this May 19, 2023

ynotdraw commented May 19, 2023

View reviewed changes

ynotdraw added 2 commits May 19, 2023 09:58

chore: Add ember/string as a dependency for tests

ad8da94

chore: Ignore glint errors for now

6ae7b64

ynotdraw commented May 22, 2023

View reviewed changes

chore: Convert tests to gts

c399620

ynotdraw force-pushed the fix-main branch from f6d6c21 to c399620 Compare May 22, 2023 18:08

ynotdraw commented May 22, 2023

View reviewed changes

ynotdraw marked this pull request as ready for review May 22, 2023 18:13

ynotdraw requested review from vitch, camskene, nicolechung, simonihmig, joelamb and noahgelmancs May 22, 2023 18:14

clintcs approved these changes May 22, 2023

View reviewed changes

simonihmig reviewed May 23, 2023

View reviewed changes

fix: Remove unneeded peerDependencies

126348c

Co-authored-by: Simon Ihmig <[email protected]>

camskene approved these changes May 24, 2023

View reviewed changes

ynotdraw mentioned this pull request May 24, 2023

Add Changesets support #84

Merged

simonihmig approved these changes May 25, 2023

View reviewed changes

ynotdraw force-pushed the fix-main branch from b10b901 to 370cb3b Compare May 25, 2023 14:28

ynotdraw merged commit a732f56 into main May 25, 2023

ynotdraw deleted the fix-main branch May 25, 2023 14:46

github-actions bot added the released label May 25, 2023

ynotdraw mentioned this pull request May 25, 2023

Update README yarn command and update link syntax #81

Merged

github-actions bot mentioned this pull request May 30, 2023

Release Preview #88

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Fix main branch typescript errors #83

chore: Fix main branch typescript errors #83

ynotdraw commented May 17, 2023 •

edited

Loading

ynotdraw May 19, 2023

NullVoxPopuli May 19, 2023

ynotdraw May 19, 2023

NullVoxPopuli May 19, 2023

ynotdraw May 22, 2023

vitch May 23, 2023

NullVoxPopuli May 23, 2023

ynotdraw May 19, 2023 •

edited

Loading

ynotdraw May 22, 2023

NullVoxPopuli May 22, 2023

ynotdraw May 22, 2023

ynotdraw May 22, 2023

ynotdraw May 22, 2023

NullVoxPopuli May 22, 2023

ynotdraw May 22, 2023

simonihmig May 23, 2023

ynotdraw May 24, 2023

simonihmig May 23, 2023

ynotdraw May 24, 2023

simonihmig May 23, 2023

ynotdraw May 24, 2023 •

edited

Loading

simonihmig May 25, 2023

simonihmig May 23, 2023

camskene left a comment

changeset-bot bot commented May 25, 2023

github-actions bot commented May 25, 2023

chore: Fix main branch typescript errors #83

chore: Fix main branch typescript errors #83

Conversation

ynotdraw commented May 17, 2023 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ynotdraw May 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ynotdraw May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camskene left a comment

Choose a reason for hiding this comment

changeset-bot bot commented May 25, 2023

⚠️ No Changeset found

github-actions bot commented May 25, 2023

ynotdraw commented May 17, 2023 •

edited

Loading

ynotdraw May 19, 2023 •

edited

Loading

ynotdraw May 24, 2023 •

edited

Loading