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 issues with v5.2.0 #503

Merged
merged 7 commits into from
May 25, 2023
Merged

Conversation

robostheimer
Copy link
Contributor

Change Log:

  • removed ember__test-helpers from package
  • bumped @ember/test-helpers to v 2.9.2 to get types directly from this package
  • Removed HookUnregister import from setup-global-a11y-hooks as it is not an exported type and updated the _unregisterHooks variable's type to match HookUnregister (Note: happy to make that HookUnregister type exportable if that makes more sense)
  • Fixed typescript error in setup-middleware-report (getContext could return undefine, however getTestMetadata expects a defined context)
  • removed types/@ember/test-helpers.d.ts since the types are available via the bumped version of @ember/test-helpers

Tests:
yarn prepublishOnly returns no errors
yarn test:ember returns

# tests 60
# pass  60
# skip  0
# todo  0
# fail  0```

Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. Needs two very small tweaks, and then we'll be off to the races!

addon-test-support/setup-global-a11y-hooks.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
addon-test-support/setup-middleware-reporter.ts Outdated Show resolved Hide resolved
robostheimer and others added 3 commits May 22, 2023 10:10
Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

Excellent, thank you!

@chriskrycho
Copy link
Contributor

@rwjblue @scalvert @drewlee this looks good for a 2.9.4 bug-fix release.

@rwjblue
Copy link
Member

rwjblue commented May 24, 2023

Unfortunately, CI was disabled (unrelated to this PR). I just fixed that.

@robostheimer - Could you rebase / amend & update to trigger a CI job?

@@ -24,6 +24,10 @@ type HelperName =
| 'typeIn'
| 'visit';

interface HookUnregister {
unregister: () => void;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering the changes in PR #1344, a 2.9.4 release of @ember/test-helpers would allow us to just import registerHook and HookUnregister as official public APIs. Is there a reason why we can't just wait for that release before making these changes? Seems like duplicative work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, I'm trying to migrate an addon to typescript; this PR unblocks that as there is an issue with a reference to ember__test-helpers. Being that the code is already written and ready for merging. Seems like the duplicative work has been done.

Do we know when the PR referenced above will be released? @drewlee

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be a question for @chriskrycho, who maintains the @ember/test-helpers releases.

Copy link
Contributor

@drewlee drewlee May 24, 2023

Choose a reason for hiding this comment

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

To be specific, the duplicative work would entail removing the inline HookUnregister type that's being introduced with this change. Just seems more efficient to incorporate these changes wholesale in accommodating a 2.9.4 release of @ember/test-helpers. We've been aware of the TypeScript bug (see issue #498) and is one of the reasons why the changes were carried out in PR #1344.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I pushed this PR at the suggestion of @chriskrycho. Chris, is there a reason why we cant just release @ember/test-helpers and then update this addon?

Copy link
Member

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

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

I agree with @drewlee that it would be better to ask @chriskrycho to please release the package with the fix in it and we can consume that. Sorry for the circles on this, and thank you for your patience! We'll get this resolved. 👍

@chriskrycho
Copy link
Contributor

One reason is: I'm super busy with other things and keeping these two things decoupled from each other is useful. :) I am happy to try to get to it, but it will be some time before I can commit to that.

@robostheimer
Copy link
Contributor Author

One reason is: I'm super busy with other things and keeping these two things decoupled from each other is useful. :) I am happy to try to get to it, but it will be some time before I can commit to that.

Gotcha @chriskrycho

Given this, @drewlee @MelSumner, do you have objections to pushing this change? cc: @rwjblue

@MelSumner
Copy link
Member

I think we should see who else on the team can release the package; I can even check since as a core framework team member I should have access. Let me check and see.

@MelSumner
Copy link
Member

One reason is: I'm super busy with other things and keeping these two things decoupled from each other is useful. :) I am happy to try to get to it, but it will be some time before I can commit to that.

no worries if you don't have time to do a release, I will find someone who can though. I have learned that it's not helpful in the long run to keep things like this limping along and I have the time and energy to see this through. 👍

@rwjblue
Copy link
Member

rwjblue commented May 25, 2023

We need to address https://github.com/emberjs/ember-test-helpers/pull/1344/files#r1205681209 before releasing a new ember-test-helpers, we should land this as is IMO.

@rwjblue rwjblue merged commit 9d6556c into ember-a11y:master May 25, 2023
@rwjblue rwjblue changed the title Fix for Issue 502 Fix type issues with v5.2.0 May 25, 2023
@rwjblue rwjblue added the bug label May 25, 2023
@robostheimer robostheimer deleted the Issue-502 branch May 25, 2023 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants