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

published version of v4.3.1 doesn't include typings or match package.json #4599

Closed
ngault opened this issue May 21, 2021 · 11 comments
Closed

Comments

@ngault
Copy link
Contributor

ngault commented May 21, 2021

The build process is not generating the directory structure described by the package.json, which indicates a Typescript project. No types are built, so the project won't work with Typescript. This makes sense to users familiar with the latest migration to Typescript, but new consumers will struggle to make sense of what's going on, especially since there is no mention of Typescript in the README, including the need to install an @types/react-select package.

From the package.json in the original source:

"types": "dist/react-select.cjs.d.ts",

From the built version:
image

@Methuselah96
Copy link
Collaborator

The line you mention has not been published yet as it's part of the upcoming v5 version which includes a conversion from Flow to TypeScript (see #4559). If you want TypeScript types for v4 you need to install @types/react-select. Here's the package.json of v4 which is the latest published version: https://github.com/JedWatson/react-select/blob/v4.x/packages/react-select/package.json.

@ngault
Copy link
Contributor Author

ngault commented May 21, 2021

@methuselah , out of respect for the community, I wouldn't close this issue before addressing it. A number of people have already needlessly wasted time trying to figure this out. A small effort on your part to follow conventions could address this.

Perhaps the new, breaking Typescript version could go in a "v5" feature branch, as opposed to living un-tag'd on MASTER, and being published as the default version to npm?

To be clear: anybody installing from npm the published version of react-select -- namely v4.3.1 -- will not know that the package.json is incorrect and that they need to find the corresponding @types version to use the Typescript. (Nor can they reasonably be expected to know that a previous "patch" version, v4.3.0, was in fact the last version before a breaking change.)

The problem starts with the fact that the HEAD of the repo is not tag'd, so devs can't be expected to understand the Typescript source code in v.4.3.1 is, as you seem to suggest, actually part of v5. Forking the current version of react-select produces a package.json with version v4.3.1 along with Typescript code.

@Methuselah96
Copy link
Collaborator

Methuselah96 commented May 21, 2021

@ngault Apologies if closing the issue was premature, but I felt like I had addressed the issue which was why I closed it.

I think maybe there's some miscommunication. v4.3.1 does not include any TypeScript. I think it's fairly typical for the HEAD of a repo to include unreleased code and for the version number to not be bumped until we go to publish. Can you clarify what your suggestion is?

@Methuselah96 Methuselah96 reopened this May 21, 2021
@ngault
Copy link
Contributor Author

ngault commented May 21, 2021

@Methuselah96 , thanks for your fast response.

I agree that it's fairly typical for the HEAD of the repo to include unreleased code. However, when the code base will undergo dramatic change (as in the breaking changes represented by the conversion to Typescript and a new set of typings that are incompatible with the @types version) the best practice is to first create a feature branch, and then merge it along with description of the breaking changes. I respect that some projects don't operate that way, but there is a larger issue here...

My suggestion is to take the point of view of the project's consumers (versus informed members). There is zero mention of Typescript in the README, much less a mention of @types/react-select. Absent this information, in 2021, any new consumer of the repo who sees only Typescript files on master will assume that the published version of that repo ships with .d.ts files, and that assumption will be be confirmed when the only package.json confirms it.

The confusion could be cleared up by stating in the README:

  • TS support comes only through @types/react-select
  • the Typescript source code on master isn't published
  • there is no previously published Typescript version
  • the incompatibilities between @types/react-select and the unpublished Typescript on master don't mean that @types/react-select is incorrect

@Methuselah96
Copy link
Collaborator

Thanks for the feedback. We do try to keep consumers informed of our future plans and the main way we do that is through pinned issues (e.g., #4559; and previously #4293 and #3910). That seems like a fairly typical way for projects to make announcements of future plans and notices to anyone looking at the project.

@JedWatson is also working on releasing a v5 beta version in the next few days which will also make it visible from the Releases page as well. Do you think a published v5 beta will be sufficient to make it clear what's going on to consumers?

Either way I'll leave it up to @JedWatson as to determine how best to proceed. We do our best to communicate to our users, but unfortunately there will always be things that slip through.

@ngault
Copy link
Contributor Author

ngault commented May 21, 2021

@Methuselah96 , by "consumers" I mean the 98% of devs using react-select that rely only on the README.md and superb documentation site, and not the 2% that, recognizing the majesty of react-select, choose it from amongst all the repos they use as having "issues" worth tracking.

Declining to take a few minutes to make a (one and only) mention of Typescript in the README, despite all the recent confusion, signals that you're concerned only about the 2%.

To answer your question, yes, I do think the release of a v5 beta version will help, but you'll still need to mention it where everybody can see it.

@thomasbruketta
Copy link

@Methuselah96 thanks for the quick answer and the clear path to resolution. @ngault that is fairly salty reply and it is not particularly constructive. A more constructive approach would be to open a PR with the update to the README you are suggesting. Also being hyper critical of public libraries when you personally have zero public repos posted feels fairly hypocritical. Be part of the solution, not the problem.

@ngault
Copy link
Contributor Author

ngault commented May 27, 2021

@thomasbruketta , some people spend most of their adult professional lives contributing to Open Source without creating a repository visible on GitHub. I'll leave it there.

As to "the clear path to resolution", sorry if I don't see that. My proposal was to provide information in the README that would prevent other consumers of the repo from the confusion that I and others here have already worked through. I had nothing to gain personally in raising the issue. Absent its resolution, more community members will needlessly waste time. I responded harshly when I perceived their interests were not being considered.

Your idea of providing a PR is a good one. When a PR involves modifying the README, the typical practice is to first propose the change to the project maintainer. The specifics of the proposal are here: #4599 (comment)
@Methuselah96 responded by saying that it was Jed Watson's decision as to what to do next, which I fully respect. I'd gladly do the PR if asked.

@Methuselah96
Copy link
Collaborator

Methuselah96 commented May 27, 2021

Declining to take a few minutes to make a (one and only) mention of Typescript in the README, despite all the recent confusion, signals that you're concerned only about the 2%.

I'm personally not convinced this is a common enough problem for me to feel inclined to create a PR to add it to the README and then remove it from the README once the TypeScript version is released. I'm not sure what "all the recent confusion" is referring to and I think it's fairly typical to check for a @types package when a library import doesn't work with an error that mentions installing the @types package as a possible solution.

My hesitancy to do such was greatly increased once sarcasm and exaggeration were introduced into the conversation. Due to those factors I deferred to Jed and stopped responding since I don't enjoy engaging in these types of discussion. Framing your view as "if you don't do this then you're concerned only about the 2%" is in my view counter-productive because now you've taken a position of attack instead of letting your arguments stand for themselves and makes the other person have to take the defensive position or submit.

In this case, if you had created a PR with your proposed changes then the suggested changes would've been much more likely to have been merged since we appreciate when people taking the time to contribute. Since it should be a trivial change (that only takes a few minutes) then there would be less wasted work to just make the PR instead of checking with the project maintainer. If it's rejected, you spent just about as much time proposing your change.

I don't know what Jed's opinion will be. I'm only explaining why I previously disengaged from the discussion and why I didn't immediately create a PR with the suggested changes.

@ngault
Copy link
Contributor Author

ngault commented May 28, 2021

@Methuselah96 here is a PR with a 1 line addition to the README that will both prevent confusion now and not require removal in the future, at least so long as there are users that need v4.

To be clear, I gladly would have taken the 5 minutes to do the PR if you hadn't rejected the specific proposal.

You make some strong criticisms, but they clearly contradict what you previously said. My mention of the "2%" that you find offensive was made only after you rejected my efforts at explaining why users were experiencing something you weren't seeing, telling me that you were not going to address it and were handing it over to Jed Watson.

I opened this issue in concert with 2 other full-time developers that had the same experience. I'll take your skepticism as reasonable and send you their names.

@Methuselah96
Copy link
Collaborator

Resolved in #4609. 🎉

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

3 participants