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: Export type of $CombinedState #4026

Closed
wants to merge 1 commit into from

Conversation

JacobLey
Copy link

@JacobLey JacobLey commented Mar 5, 2021


name: Export type of $CombinedState
about: Support latest version of Typescript

PR Type

Does this PR add a new feature, or fix a bug?

Fix a bug with lastest version of Typescript

Why should this PR be included?

Exporting the result of combineReducers currently fails with latest version of Typescript, requires access to $CombinedState.

See Bug Fixed -> What is the current behavior, and the steps to reproduce the issue? for in-depth explanation

Only the type is exported, so it is not possible a consumer thinks the value is actually usable (no risk)

Checklist

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Is there an existing issue for this PR?
    • None, but can create if needs to be discussed further
  • Have the files been linted and formatted?
  • Have the docs been updated to match the changes in the PR?
  • Have the tests been updated to match the changes in the PR?
  • Have you run the tests locally to confirm they pass?

New Features

Typeof $CombinedState is exported an available to consumers

What new capabilities does this PR add?

Adds support for Typescript exporting the result of combineReducers with latest version of typescript

What docs changes are needed to explain this?

None

Bug Fixes

What is the current behavior, and the steps to reproduce the issue?

Upgrade to typescript v2.4.2

Create a file example.ts with the following content:

import { combineReducers, $CombinedState } from 'redux';

export default combineReducers({})

Typescript will emit an error on:
Default export of the module has or is using private name '$CombinedState'.

What is the expected behavior?

No error should be emitted

How does this PR fix the problem?

By explicitly exporting the typeof $CombinedState, Typescript will be able to properly handle an exported combineReducers

Admittedly this mostly feels like an issue with Typescript (the type is already available to import) but it is an otherwise safe, non-breaking change so it is worth fixing at the Redux level

@netlify
Copy link

netlify bot commented Mar 5, 2021

Deploy preview for redux-docs ready!

Built with commit 1284f1b

https://deploy-preview-4026--redux-docs.netlify.app

@markerikson
Copy link
Contributor

The first issue here is that this PR won't actually fix anything atm. master is currently an unreleased conversion of the Redux core to TS that will (maybe, eventually, someday) become Redux 5.0.

The current version is 4.0.5, which is still written in JS with attached typedefs. That's on the 4.x branch.

That said, I don't fully understand what the problem is, and would appreciate seeing an issue filed with a reproduction.

@timdorr
Copy link
Member

timdorr commented Mar 6, 2021

Hmm, is this something that changed in TS 4.2? I'm not seeing any reference to it. Our own internal use of these types doesn't produce these kinds of errors.

It sounds like something is wrong with your build environment. Exporting a private symbol (which is only to ensure a distinct type for inference purposes) shouldn't be needed here, nor is it desired because we don't want folks relying on it or messing with it in some way. This isn't part of our API surface.

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