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

feat(compat): update patch for typescript 4.4 #3297

Merged
merged 10 commits into from
Aug 31, 2021
Merged

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Aug 15, 2021

What's the problem this PR addresses?

  • The PnP patch for TypeScript doesn't apply to typescript@>=4.4.1-rc
  • To quote the test file:
The PnP patch returned the packageLocation with a trailing `/` which
caused TypeScript to calculate the `subModuleName` incorrectly[1].
This caused different files where only the first character of their
basename was different to end up with the same packageIdKey[2].
In this test the affected files were
@types/lodash-es/find.d.ts -> @types/lodash-es/ind.d.ts@4.17.4
@types/lodash-es/bind.d.ts -> @types/lodash-es/ind.d.ts@4.17.4

1: https://github.com/microsoft/TypeScript/blob/98866a5a740b487c20046d4ffaa36aa1f202dde9/src/compiler/moduleNameResolver.ts#L20
2: https://github.com/arcanis/TypeScript/blob/36225c32609137d29008e9d3ecf48c6f51456eb5/src/compiler/program.ts#L2625
  • The references patch doesn't preserve paths when resolving virtuals and assumes everything points to a package root

Fixes #3058
Fixes #3231
Fixes #3117
Fixes #3350
Fixes #3262
Closes #3275
Closes arcanis/TypeScript#10

How did you fix it?

Checklist

  • I have read the Contributing Guide.
  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@merceyz merceyz requested a review from arcanis August 15, 2021 22:31
@merceyz merceyz changed the title feat(compat): update patch for typescript 4.4 fix(compat): update patch for typescript 4.4 Aug 15, 2021
@merceyz merceyz changed the title fix(compat): update patch for typescript 4.4 feat(compat): update patch for typescript 4.4 Aug 15, 2021
@merceyz
Copy link
Member Author

merceyz commented Aug 19, 2021

Failing CI will be fixed by #3315

@sushruth
Copy link

Looking forward to this fix! this helps many projects based on nx.

There was no conflict but updated the patch either way
cmfcmf added a commit to roc-and-roll/roc-and-roll that referenced this pull request Aug 28, 2021
@arcanis arcanis merged commit ffe1a64 into master Aug 31, 2021
@arcanis arcanis deleted the merceyz/fix/typescript branch August 31, 2021 11:28
@SimenB
Copy link

SimenB commented Sep 1, 2021

Will this be backported to v2? I have a bunch of projects which still supports node 10

@sushruth
Copy link

sushruth commented Sep 2, 2021

This may have broken some base scenarios - https://github.com/sushruth/simple

here is a simple repro repo which imports react and console logs it. TSC finds that erroneous like so -

image

@merceyz
Copy link
Member Author

merceyz commented Sep 2, 2021

It didn't, see #3350 (comment)

@sushruth
Copy link

sushruth commented Sep 2, 2021

@merceyz still fails. It fails for all packages which have typings included too. Added immer to show this -
image

@merceyz
Copy link
Member Author

merceyz commented Sep 2, 2021

Read the rest of the comments in the issue I linked please

@sushruth
Copy link

sushruth commented Sep 3, 2021

@merceyz my bad. Thanks!

arcanis pushed a commit that referenced this pull request Sep 3, 2021
* fix(compat): remove trailing slash

* fix(compat): update patch for 4.4

* test: add testcase

* chore: versions

* fix: handle references that doesn't point to a package root

* fix: backport references fix to 4.2

* chore: dedupe

* fix: avoid direct requires to `pnpapi`

* chore: reuse files in validation

* chore: update to `4.4.2`

There was no conflict but updated the patch either way
arcanis pushed a commit that referenced this pull request Sep 3, 2021
* fix(compat): remove trailing slash

* fix(compat): update patch for 4.4

* test: add testcase

* chore: versions

* fix: handle references that doesn't point to a package root

* fix: backport references fix to 4.2

* chore: dedupe

* fix: avoid direct requires to `pnpapi`

* chore: reuse files in validation

* chore: update to `4.4.2`

There was no conflict but updated the patch either way
@arcanis
Copy link
Member

arcanis commented Sep 3, 2021

Will this be backported to v2? I have a bunch of projects which still supports node 10

@SimenB yes, I'm in the process of cherry-picking it on 2.4; I'll have to afk, but I'll make sure it gets released tomorrow.

@SimenB
Copy link

SimenB commented Sep 3, 2021

Awesome, thanks! 🎉

@arcanis
Copy link
Member

arcanis commented Sep 6, 2021

Released 2.4.3

@SimenB
Copy link

SimenB commented Sep 6, 2021

Thanks! Can confirm it works 👍

@mrienstra
Copy link
Contributor

These changes were released in 3.0.2: @yarnpkg/cli/3.0.2, @yarnpkg/pnp/3.0.2, @yarnpkg/plugin-pnp/3.0.2.
(Old news, just adding for the convenience of others)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment