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: disallow typescript@4.8 peer dep #8443

Closed
wants to merge 1 commit into from
Closed

Conversation

levino
Copy link

@levino levino commented Sep 5, 2022

remove typescript@4.8 from the peer dependency range
since typescript@4.8 is incompatible with parcel at this time.
make sure we use typescript 4.7 for development everywhere.

See #8419 (comment)

@@ -15,7 +15,7 @@ jobs:
- uses: Swatinem/rust-cache@v1
# use `--frozen-lockfile` to fail immediately if the committed yarn.lock needs updates
# https://yarnpkg.com/lang/en/docs/cli/install/#toc-yarn-install-frozen-lockfile
- run: yarn --frozen-lockfile
Copy link
Author

@levino levino Sep 5, 2022

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

'Important: This documentation covers modern versions of Yarn. For 1.x docs, see classic.yarnpkg.com.'

We are using Yarn 1 here

Copy link
Author

Choose a reason for hiding this comment

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

I did not know that yarn@1 behaves so poorly. Thanks for the update. I removed this part of the PR. However I recommend using yarn@3 soonish, it is much better than yarn@1.

@@ -50,6 +50,6 @@
"@typescript-eslint/parser": "^4.14.1",
"eslint": "^7.19.0",
"glob": "^7.1.6",
"typescript": "^4.6.4"
"typescript": "~4.7"
Copy link
Author

Choose a reason for hiding this comment

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

These should be pinned too. If not they would use typescript@4.8 and having two ts versions installed will only cause problems. I would even recommend installing typescript as a dev dependency only in the root package.json and not in any of the packages. Would make things easier.

},
"peerDependencies": {
"typescript": ">=3.0.0"
"typescript": "3.0.0 - 4.7"
Copy link
Author

@levino levino Sep 5, 2022

Choose a reason for hiding this comment

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

I personally would immediately discontinue support for typesscript@3. Iheavily doubt that typescript@3 is really compatible with current parcel. There are no tests run with typescript@3 so I would say: Nobody knows for sure. Instead of creating a test matrix with older typescript versions, I would rather just discontinue support for typescript@3.

remove typescript@4.8 from the peer dependency range
since typescript@4.8 is incompatible with parcel at this time.
make sure we use typescript 4.7 for development everywhere.
@devongovett
Copy link
Member

I don't think this will work. The dependency is a peerDependency, so the actual version that will be used comes from the user's project.

  • With yarn, the version in the project will win, and 4.7 won't get installed. With npm, both versions will be installed, which might also break things if there are incompatibilities.
  • This raises the required version from 3.0 to 4.7, so it will break anyone using an older version.

We could potentially error at runtime if the version is greater than expected, but this would not scale since breaking changes are somewhat rare and TS releases new versions all the time. We wouldn't want to have to manually bump the maximum version each time before it would work.

I think the only solution is to actually fix whatever is broken. We might have to add some forked codepath depending on what version is loaded, for example.

@levino
Copy link
Author

levino commented Sep 7, 2022

I don't think this will work. The dependency is a peerDependency, so the actual version that will be used comes from the user's project.

  • With yarn, the version in the project will win, and 4.7 won't get installed. With npm, both versions will be installed, which might also break things if there are incompatibilities.

No. If someone uses say typescript@4.8 in their project yarn will show a warning during install: Unmet peer dependency for parcel: typescript@3.0 - 4.7. This is how peer dependencies work: You can explain to users with which version of an installed package your package is compatible. As of now parcel is simply incompatible with typescript@>=4.8

  • This raises the required version from 3.0 to 4.7, so it will break anyone using an older version.

No. I just set the devDependency to typescript@~4.7. This means that when you develop parcel you will use typescript@4.7. As you can see from the changes in the yarn.lock before the used version was typescript@4.6. But if you do not restrict the devDependency to typescript@~4.7 on the next rewrite of the yarn.lock (which is totally fine), typescript@4.8 (or higher) would get installed and tests will break.

We could potentially error at runtime if the version is greater than expected, but this would not scale since breaking changes are somewhat rare and TS releases new versions all the time. We wouldn't want to have to manually bump the maximum version each time before it would work.

That is not necessary and dangerous. Runtime checks of this kind always back fire. Manually changing the typescript version is totally fine and doable. You then also can run all your tests with the new typescript version and make sure that nothing breaks. A totally feasible approach. Nobody needs to be able to use parcel with typescript@4.9 the same day typescript@4.9 is released. Better be safe than sorry!

I think the only solution is to actually fix whatever is broken. We might have to add some forked codepath depending on what version is loaded, for example.

That is a next step. Of course the incompatibility issues have to be resolved. But in their own time, not under stress and with the awful reality, that until then everybody using parcel and typescript in a new project has a very poor experience.

@devongovett
Copy link
Member

devongovett commented Sep 7, 2022

Say someone installs typescript 4.8 in their project. With yarn, they'll get a warning that parcel is not compatible but it doesn't prevent install from succeeding. Many projects have lots of these warnings so it might get lost. When running the build, it'll load 4.8 and the user will see the same error as today. Doesn't really solve the problem.

With npm, both 4.7 and 4.8 will be installed. Parcel will then load 4.7, but building against a project using 4.8. Might work, but also might break in different ways. But no obvious error about why.

I don't want to have to manually bump and test every new version of typescript before anyone can use them with parcel. Users should have control over this in their projects. Right now if they bump to 4.8 and it doesn't work, they can just downgrade, and report the issue to us to fix. But if we limit the dep to 4.7, then they can't upgrade at all until we release a new version. They aren't in control. Since actual breaking changes in typescript that affect parcel are rare, I think this is generally a better system because it doesn't block projects from upgrading.

@levino
Copy link
Author

levino commented Sep 7, 2022

I do not use npm but I thought they stopped installing the peer dependencies automatically with npm@3. So the behaviour would be the same as with yarn, right? They discourage this very strongly and say

Use of legacy-peer-deps is not recommended, as it will not enforce the peerDependencies contract that meta-dependencies may rely on.

I did not make up the system with the peer dependencies. It makes sense and should be used exactly how I described it above. Ignoring the warnings that you have unmet peer dependencies during a yarn or npm install is dangerous and bad practice. I do not accept this as an argument. parcel will cause problems for people who just stick to the best practices. I heavily doubt that with webpack and rollup things are as laissez-faire. It is just a question of (perceived) quality. After all, you are making the calls here, but this decision to "hope for the best" and "let the users figure it out, at least we get the bug reports then" makes parcel a time bomb for many projects.

@devongovett
Copy link
Member

As of npm 7, npm will again automatically install peer dependencies just like normal dependencies.

I really don't think it's as bad as you are making it out to be. These days, most people use lock files, so there won't be upgrades unless users opt-in. And if it doesn't work, it's easy to roll back.

To be completely honest, this is 100% TypeScript's fault. They don't follow semantic versioning, but yet that's how all package managers work so they don't really have a choice. When they release breaking changes, they should bump the major version. Anything else just leaves projects using the typescript compiler like parcel in a very tight spot where there really isn't a good solution. I would recommend expressing your displeasure with TypeScript instead.

@devongovett
Copy link
Member

@levino
Copy link
Author

levino commented Sep 8, 2022

The question is: Do webpack and parcel check if things break? It does not matter what exactly the contract the package is providing says (either "use all typescript versions" or "use only these typescript versions") as long as the project lives up to it and the code really is compatible. I still do not see any issue in going out there and telling everyone: "If you use typescript@4.8 there are potentially issues! Better use typescript@4.7" when they install the package (actually there are known issues with typescript@4.8!). Instead people have to skim through the github issues now, to get this very same information. It is just more cumbersome. I am just trying to make your life easier and your users happier here.

And you are right that typescript is sloppy with the versioning. However it seems to me that this is known to many people. Also to a certain degree it makes sense, because if they would do it "correctly", we would maybe have typescript version 243 or something. Also I see no point in talking with typescript about release management, because this really would be a change in their methodology. Here at parcel, I just propose a change that can be accepted once via "approve PR" and that is that. No change to your methodology. You even say "people usually ignore that". So nothing changes but for the people using npm with the peed dependency auto install. There I have to say: "Who uses npm for installs after all? It is kind of obsolete since yarn came around..." And if you routinely automatically install peer dependency, you will be very used to the fact that you have very problematic double installs of some packages (the worst of it I guess will be react and @types/react in multiple versions, good luck with that). So these people just like to live on the edge...

And for the yarn.lock argument: I knew this was coming, but I am sorry to say but in my understanding you are thinking of this file in the wrong way. It is not supposed to pin your versions and allow you to "carefully opt into upgrades". You can kind of use it in that way, but actually this is what resolutions is for. It is okay to always delete and recreate the yarn.lock at any time. Your project should not care. You get all security upgrades, bug fixes etc., but your project should behave the same. Its sole purpose should be to transfer the exact development environment from one developer machine to another developer machine so bugs are reproducible. it is not designed to prevent users from using the latest compatible version of a dependency. This is obvious from the fact that even if you push your yarn.lock during a release to npmjs.org, it will be ignored when your package gets installed as a dependency - if I am not utterly mistaken here. So this is why we currently have

    "resolutions": {
        "typescript": "~4.7",
        "typescript:comment": "see https://github.com/parcel-bundler/parcel/issues/8419"
    },

in our root package.json. If parcel would declare typescript@4.7 a peer dependency, we would have:

    "dependencies": {
      "typescript": "~4.7"
    },

I admit that a lot of maintainers regularly publish breaking changes as minor bumps. That is a sad reality with npmjs.org. But one can quickly pin point these bad apples and put them all in the resolutions field. It is not a good idea imo to pass on all security updates for all of your potentially thousands of dependencies, just because you have a couple of badly maintained packages in that list.

But whatever. We agree to disagree. I respect your opinion and I really like parcel much better than webpack and rollup. You people are doing a great job here overall. So thank for taking the time considering my PR and also the in depth feed back and research you put into this. I wish you all a pleasant day and maybe I have a more acceptable contribution to make next time. Take care!

@levino levino closed this Sep 8, 2022
@levino
Copy link
Author

levino commented Sep 8, 2022

To be completely honest, this is 100% TypeScript's fault. They don't follow semantic versioning, but yet that's how all package managers work so they don't really have a choice. When they release breaking changes, they should bump the major version. Anything else just leaves projects using the typescript compiler like parcel in a very tight spot where there really isn't a good solution. I would recommend expressing your displeasure with TypeScript instead.

BTW: What you say here would only be true iff you had

"peerDependencies": {
    "typescript": "3 - 4"
}

in your package.jsons. Even if typescript would be "better" with versioning, you still would have to declare that typescript@5 is "probably not compatible". In the end it does not matter how typescript handles their releases, the problem here would be exactly the same.

@devongovett
Copy link
Member

Yeah but the problem is 4.8 might be breaking or it might not be - the version number is meaningless. I don't have the time to manually bump and test typescript and release a new parcel version every time they do a release. 99% of the time there are no breaking changes, so this is a waste of my time mostly. If typescript followed semantic versioning, then I'd know which ones were important to test and which ones projects could upgrade on their own.

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