Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix type violations due to missing glint configuration #90

Merged

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Mar 16, 2023

Many of the type errors that I could see were due to missing the template-imports glint environment.
This has been added.

@changeset-bot
Copy link

changeset-bot bot commented Mar 16, 2023

⚠️ No Changeset found

Latest commit: 8bf8253

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.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

Copy link
Contributor

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for looking into this!

CI shows some new issues though, any idea? https://github.com/CrowdStrike/ember-headless-form/actions/runs/4439238826/jobs/7791450494?pr=90#step:5:27

@@ -33,7 +33,6 @@
"@embroider/util": "^1.10.0",
"ember-async-data": "^0.7.0",
"ember-modifier": "^4.1.0",
"rollup-plugin-glimmer-template-tag": "^0.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oups, that was a dependency? 🤦

@@ -187,9 +188,11 @@ export default class HeadlessFormComponent<
DATA extends UserData,
SUBMISSION_VALUE
> extends Component<HeadlessFormComponentSignature<DATA, SUBMISSION_VALUE>> {
// we cannot use (modifier "on") directly in the template due to https://github.com/emberjs/ember.js/issues/19869
on = on;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. I was too eager to remove this 👍

@@ -1,12 +1,13 @@
import Component from '@glimmer/component';
import { tracked } from '@glimmer/tracking';
import { assert, warn } from '@ember/debug';
import { hash } from '@ember/helper';
// @ts-ignore hash is missing from ember types
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But linting was happy with this file. IIRC modifier was actually missing, not? But somehow it was not needed, the template was still working for me locally without the import 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it's a built-in, but not yet for types -- I think there is an open issue about it and it came up in discord a couple days ago

@NullVoxPopuli
Copy link
Contributor Author

some new issues though, any idea?

potentially!

it's possible something got messed up with this code: https://github.com/NullVoxPopuli/rollup-plugin-glimmer-template-tag/blob/main/packages/rollup-plugin-glimmer-template-tag/src/rollup-plugin.js#L90 -- it relies on the consumer's babel config to be able to parse correctly

@NullVoxPopuli
Copy link
Contributor Author

idk what's up with the lints (out of date dep, props?)
but everything else is green. lemme know if you want help with the lints.

@NullVoxPopuli
Copy link
Contributor Author

Looks like:

Unsafe dynamic component: HeadlessFormFieldComponent in $TMPDIR/embroider/fd7bfa/node_modules/.pnpm/file+packages+ember-headless-form_yj3yrsfg6wbq6iy2hki53xivum/node_modules/ember-headless-form/dist/components/headless-form.js

@simonihmig
Copy link
Contributor

I'll merge this as-is, and try to fix the remaining issues in the main branch!
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants