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

(webdriverio): remove type exports #11590

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

christian-bromann
Copy link
Member

Proposed changes

We currently export all WebdriverIO types from the module, including the ones for Browser and Element. These are not meant to be used for public consumption, instead folks should use the WebdriverIO namespace, e.g. WebdriverIO.Browser.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Further comments

fixes #11055

Reviewers: @webdriverio/project-committers

@christian-bromann christian-bromann added the PR: Bug Fix 🐛 PRs that contain bug fixes label Nov 6, 2023
@christian-bromann christian-bromann merged commit 94a6a59 into main Nov 6, 2023
3 checks passed
@christian-bromann christian-bromann deleted the cb/webdriverio-type-export branch November 6, 2023 03:01
@dbjorge
Copy link

dbjorge commented Nov 6, 2023

@christian-bromann This is a breaking change for folks that used the previously-exported types. I definitely understand the desire to avoid multiple definitions and think it's reasonable to collapse down to 1 exposed set of typings, but it should be done in a breaking change release, not a bugfix.

@christian-bromann
Copy link
Member Author

I disagree this being a breaking change since we never documented nor advertised the use of these types.

@dbjorge
Copy link

dbjorge commented Nov 6, 2023

Of course, it's your project and it's your prerogative to decide what should constitute a breaking change for it, but please understand that many reasonable folks will consider "a type that is exported from the package's index" to be an advertised part of the package's API, regardless of explicit documentation about it. Realistically, more folks discover types through IDE auto-completion than through documentation.

In actual practice, you can find 1000+ cases that this would break between the following two GitHub code search queries (which are not exhaustive):

Is it truly so important to get this change in now, rather than waiting for the next major version, that it's worth breaking 1000+ known usages?

@christian-bromann
Copy link
Member Author

@dbjorge you are making a good point! I am surprised it is used that much. I reverted the change and added a deprecation warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bug Fix 🐛 PRs that contain bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Browser type primitive mismatches WebdriverIO.Browser global type
2 participants