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: typescript interface updates #973

Merged
merged 4 commits into from
Jun 26, 2018
Merged

fix: typescript interface updates #973

merged 4 commits into from
Jun 26, 2018

Conversation

jeeyyy
Copy link
Contributor

@jeeyyy jeeyyy commented Jun 26, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

Description of the changes

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

axe.d.ts Outdated

type resultGroups = "inapplicable" | "passes" | "incomplete" | "violations";
type ResultGroups = "inapplicable" | "passes" | "incomplete" | "violations";
Copy link
Member

Choose a reason for hiding this comment

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

Should this be an enum? That way we get better intellisense/etc

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly, when this definition was first written support for enums wasn't good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right @marcysutton enums got introduced in ts 2.4 I believe

Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

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

One quick question about ResultGroups, but this LGTM! Thanks for the quick fix 💯

axe.d.ts Outdated

type resultGroups = "inapplicable" | "passes" | "incomplete" | "violations";
type ResultGroups = "inapplicable" | "passes" | "incomplete" | "violations";

type RunOnlyObject = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a confusing name if it isn't being used in runOnly (only in ElementContext)

Copy link
Member

Choose a reason for hiding this comment

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

It'd be a breaking change to rename it tho, right?

axe.d.ts Outdated

type TagValue = "wcag2a" | "wcag2aa" | "section508" | "best-practice";
enum TagValue {
wcag2a = "wcag2a",
Copy link
Member

Choose a reason for hiding this comment

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

This will break users' code if they were previously using TagValues.

For example, we're no longer able to call axewebdriverjs#withTags(['wcag2a']), but instead, will need to do axewebdriverjs#withTags([ axe.TagValue.wcag2a ]).

Are we comfortable making a breaking change here?

Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

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

The move to enums is a breaking change. We either need to be comfortable with a new major release, or we need to figure out a different solution.

@jeeyyy
Copy link
Contributor Author

jeeyyy commented Jun 26, 2018

@stephenmathieson
You are right, we cannot do the enum change as much as we would like it, it is a breaking change.

But in my experience, that is the idea of typings.
With every major release interfaces/ object structure does change and the availability of typings only make it easy for the end user to adapt to the newer change.

I have rolled back to type and just do the fix you are after.

Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

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

👍

@jeeyyy jeeyyy dismissed stephenmathieson’s stale review June 26, 2018 18:50

Reverted enum changes, and fixed runOnly interface

@jeeyyy jeeyyy merged commit f8c9905 into develop Jun 26, 2018
@jeeyyy jeeyyy deleted the ts-interface-runOnly branch June 26, 2018 18:51
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.

3 participants