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

[suggestion] maintain 'redux-starter-kit' npm package until version 2. #258

Closed
jmccarrell-lmi opened this issue Nov 19, 2019 · 14 comments
Closed

Comments

@jmccarrell-lmi
Copy link

My company is revolting since the name change, because they feel they can't trust redux starter kit. This combined with the lack of enthusiasm for typescript has made things tense.

I get it, I like to move fast and break things and I hate typescript, but many companies including mine are adament in using typescript, no matter how much it slows down their operations.

Anyways... I don't blame you for the typing lacking, but I highly suggest maintaining the redux-starter-kit npm package as a mirror of 'redux-toolkit' until version 2. It's a small thing that will likely restore a lot of confidence in the project.

@markerikson
Copy link
Collaborator

Uh... what do you mean "can't trust Redux Starter Kit"? Can you give more details?

I do apologize for the name change occurring after 1.0, but it was really only after 1.0 came out that we started getting a lot of feedback that the "Starter" portion of the name was confusing. See #246 for the discussion on that.

Also, just like any other lib written in TS, you can use RTK in either plain JS or TS in your own app. See the "Intermediate Tutorial" page for examples of using it from JS, and the "Advanced Tutorial" for examples of using it from TS.

@phryneas
Copy link
Member

Adding to that, if you have specific complaints against the typings, please report them here. Nobody is helped by you calling the types "lacking" without any additional information. What do you want to achieve and what isn't working?
This library is written in TypeScript and consists of about three times more typings than actual code, so you might assume we really want those to work.

@jmccarrell-lmi
Copy link
Author

jmccarrell-lmi commented Nov 22, 2019

When my team pushed redux starter kit on to the wider company as a recommendation, they adopted it (also for some reason this meant everyone had to change all of their code to use as much of RSK as possible, i don't get it.), but that was just before the release which changed the slice property to name, triggering a lot of changes in the mono-repo they were maintaining. Shortly after that, the whole package name changed.

This meant updating the dependency on their mono-repo.

Honestly, I don't understand the confidence thing, I'm just trying to help my work mates be less scared of change.

If it's too difficult to maintain an npm package with the same code and versions, but with the old name, then don't worry about it.

@jmccarrell-lmi
Copy link
Author

jmccarrell-lmi commented Nov 22, 2019

They also believe that Redux-Starter-Kit considers Typescript an unimportant side effort that won't be given very strong maintenance. (Sorry, I don't have more detail. I think they were going off of a github issue that once stated typescript was a lower priority for RSK or some such.)

EDIT: it's regarding a blog post about the release of 1.0.0

@phryneas
Copy link
Member

phryneas commented Nov 22, 2019

That slice name change happened in version 0.8. That was a pre-1.0 version.
While RTK was essentially bug-free even at that time (and was perfectly fine for production use from that point of view), that version number was there to communcate that there WOULD be api changes at any given moment.

To quote the semantic versioning specification

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

It is now in the 1.y.z range, so you can expect a stable api going forward. Any breaking changes will trigger a major version update, so you're safe there.

As for the TypeScript part, RTK was initially written in JavaScript, but migrated over to TypeScript in early 2019. From that point on, TypeScript was always a priority.
The new style guide for Redux marks both using Redux Toolkit and using TypeScript both as STRONGLY RECOMMENDED, so you can point your colleagues to that to assure them that TypeScript indeed is a priority.

The name change itself is unfortunate, but it had to be done as many people were ignoring RSK, assuming it was meant for beginners because of the "starter" in the name.
Keeping both names alive in parallel would lead to blog articles, tutorials etc. being written with both names in use, segregating the ecosystem even further.

Even in a large monorepo, a hard search&replace from 'redux-starter-kit' to '@reduxjs/toolkit' should be done in seconds. It's unfortunate, but it's the only right thing Mark could do. Sorry!

If your colleagues have further doubts, you could direct them to this issue - if they have any further doubts, I'm sure we can answer their questions.

@jmccarrell-lmi
Copy link
Author

Thanks, that's great.

So in found some comments in the source that sorta flies against the docs:

    // @ts-ignore createNextState() produces an Immutable<Draft<S>> rather
    // than an Immutable<S>, and TypeScript cannot find out how to reconcile
    // these two types.

What this means is that when we get a typing error, it's hard to know if we're at fault, or if RTK is at fault.

For example we're currently getting an error with Exported type alias 'MeetingsAction' has or is using private name 'CalActions'. I can spend a few hours trying to reproduce on a clean repo, but i have co-workers assuming that RTK is to blame and with ts-ignores littered through out the repo, it's hard to convince them they should trust RTK.

@phryneas
Copy link
Member

phryneas commented Nov 22, 2019

Adding @ts-ignores, or casting to any within a function body are totally fine in every code base, given that there is a good reason for it (see the comment you quoted) and code is tested extra-carefully.
When you look at our codebase, you will see that there are tests for the functionality and even additional tests for our typings. Our tests run in CI for every TypeScript version starting at 3.4 up until the release candidate for the next version of TypeScript.

In this specific case, we have to work around a limitation of TypeScript. If we wouldn't do that @ts-ignore, we could not offer type definitions for our API that provide the level of IntelliSense they currently are.

All typings within function bodies are removed at compile time.
That means that these either are a typing error at our compile time, or they will never be able to cause an error for you.

All the typings relevant to you as a library consumer are https://unpkg.com/browse/@reduxjs/toolkit@1.0.4/dist/index.d.ts

@jmccarrell-lmi
Copy link
Author

image (1)
This is another reason they're up in arms.

@phryneas
Copy link
Member

So. Is there any current feature that is, from your point of view, lacking in types? If so, please report it and we can try to make it better.

What you cited there might just mean that a future feature might have "less-than-ideal" typing, but not that an existing feature might at any point get "worse" typing that it currently has. And of course, for everything in the future, we'll try our best.

But of course, there might be a point where we reach the limitations for what TS can express and we'll have to make a choice of shipping the future with "good effort"-types, or not ship it to any user at all. But that is a situation literally every library you are using might face at some time, and it's a decision that will be done then.

@markerikson
Copy link
Collaborator

markerikson commented Nov 22, 2019

@jmccarrell-lmi : yes, I wrote that specific paragraph.

The point there is not that I'm against using TS, or that RTK won't support TS.

What it means is that I've focused on designing APIs that are more JS/DX-friendly, rather than TS-centric.

As a specific example, createSlice's use of an object map and string concatenation to generate the action types results in action types that are of type string instead of type "exact/literal". This may matter in some rare cases, but the point is that the object definition form is the simplest API for users to work with.

I find it hard to understand why people would be "up in arms" about that particular paragraph. Can you clarify what specific concerns they have? Like, what are people literally saying about that?

As you can see by looking at the code, we've gone to great lengths to try to set up good types for RTK, and to maximize the amount of inference while minimizing the number of places you actually have to define types. If we didn't care about TS at all, we wouldn't have done that.

I'll be honest and say that obsession over "perfect typings" is my biggest complaint about the TS community.

TS is a tool. It has strengths, weaknesses, and tradeoffs. Part of that is that since JS is a dynamic language, there are limitations in just how much of that dynamic behavior can be expressed in static types. As a general observation, it feels like many folks in the TS community are more obsessed over writing fancy type definitions than they are shipping meaningful working code, and that having to add a couple extra type declarations in places to make things line up is regarded as a failure of "type safety" somehow.

I'm a pragmatist. I want good types as much as possible, but I also want to actually write my app code and move on with my life without spending multiple hours trying to get some insanely complex type incantation to satisfy the compiler. I'm totally happy to throw a type FixTypeLater = any in the middle of my code if it lets me move on and get my job done.

So, similarly, as a lib maintainer, my goal is the 90% use case. I want to provide a good DX for both plain JS and TS users, and I do want our types to work well, but I'm not going to radically change RTK's API just to satisfy some TS-only edge cases.

Now, I am open to anything that reasonably improves our type handling. @phryneas was tossing around the idea of a "reducer builder" API that would be more type-safe for cases like the extraReducers option ( https://twitter.com/phry/status/1196874866407989248 ), and I'd be willing to add that even though it only has benefits for TS users, because it doesn't require modifications to the core APIs.

@markerikson
Copy link
Collaborator

And fwiw, I 100% agree with everything @phryneas has said in this thread, especially the comments about wanting to improve the types and the justifications for the API/name changes.

@jmccarrell-lmi
Copy link
Author

Ya sorry about the heavy words. I have a book about using more tactful language, but i have yet to read it ;)

What you've written here is EXACTLY what i was looking for. I don't know if it'll put their minds at ease, but it's put my mind at ease.

In my opinion, if people want types that can't easily be provided by RTK, then they could add a supporting library, or if they have a clean resolution, then they could do a PR on RTK.

Sorry for the negativity. RTK has been one of the rare imports I've been excited about since I discovered redux yield effect and immer JS and redux itsself. Thanks for the brilliant toolkit.

@markerikson
Copy link
Collaborator

Yeah, as a maintainer it's kind of hard to come up with a good response to vague "other folks aren't happy" comments.

As @phryneas and I both said, we're totally happy to try to improve any of the types where possible, we're just trying to find a balance in API design.

Per the original question, no, I'm not planning to simultaneously maintain RSK and RTK. The only other release I plan to make for RSK will likely be a 2.0 that is an empty shell of a package and just has throw new Error("Please update to @reduxjs/toolkit"), similar to what React Testing Library already did.

Sounds like we've answered things at this point, so I'll go ahead and close this.

@trusktr
Copy link

trusktr commented Dec 21, 2019

Regarding the all-too-common "has or is using private name" error above, I've opened a request to fix issues like these in TypeScript by bringing declaration files to parity with language features. microsoft/TypeScript#35822

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

No branches or pull requests

4 participants