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

Add export fields to manifests to prevent deep imports #4834

Merged
merged 11 commits into from
Sep 15, 2022

Conversation

Larry1123
Copy link
Contributor

@Larry1123 Larry1123 commented Sep 8, 2022

Todo:

  • fix anything that can't resolve (likely test).
  • Settle on what to do with types condition

What's the problem this PR addresses?

This is help with 4.x as requested in #3591
This addresses the task Add export fields to our manifests to prevent deep imports

How did you fix it?

Added exports to all manifests.
Added exports for some things that were not exported before.

Checklist

  • 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.

Also made sure to export some utils that may not have been exported before.
@Larry1123 Larry1123 requested a review from arcanis as a code owner September 8, 2022 10:10
@merceyz merceyz marked this pull request as draft September 8, 2022 10:10
@merceyz merceyz changed the title Draft: Add export fields to manifests to prevent deep imports Add export fields to manifests to prevent deep imports Sep 8, 2022
@merceyz
Copy link
Member

merceyz commented Sep 8, 2022

I converted the PR to a draft since that's what the title said it was.

@Larry1123
Copy link
Contributor Author

Thanks I tried to look for the button for that but I guess I just missed it.

@Larry1123
Copy link
Contributor Author

I am not too sure how to fix scripts/test-typescript-patch.js
Some help with that would be nice.

@Larry1123
Copy link
Contributor Author

I think that packages/acceptance-tests/pkg-tests-specs/sources/pnp-esm.test.ts also may need someone that knows the @yarnpkg/pnp package better judge how best to handle fixing the import of it in this test.
I don't clearly see that this loaderFlags should be exported so I am not too sure how to address this.

I tried adding it only to the exports field and not the publishConfig.exports but I'm not sure if that is the right way to go about this.

@merceyz merceyz self-requested a review September 8, 2022 14:33
@Larry1123 Larry1123 marked this pull request as ready for review September 8, 2022 15:57
@Larry1123
Copy link
Contributor Author

The only real question I have left for this PR is was flagging all affected packages as a major change right? I figured as it blocks deep imports that it likely should be counted as such.

Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

According to https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-7.html#packagejson-exports-imports-and-self-referencing we don't need the types condition since the .d.ts file is next to the .js file.

The only real question I have left for this PR is was flagging all affected packages as a major change right? I figured as it blocks deep imports that it likely should be counted as such.

Yeah, this is a breaking change so major is correct.

packages/yarnpkg-libzip/package.json Outdated Show resolved Hide resolved
packages/yarnpkg-libzip/package.json Outdated Show resolved Hide resolved
packages/yarnpkg-libzip/package.json Show resolved Hide resolved
@merceyz merceyz mentioned this pull request Sep 8, 2022
13 tasks
removed `./sync` and `./async` from `@yarnpkg/libzip`
Normalized some Command Class names.
@Larry1123
Copy link
Contributor Author

Larry1123 commented Sep 8, 2022

I have had issues when the types condition is omitted, I will confirm firsthand that it will be fine to omit the condition before removing them. In most manifests there were types or typeings in publishConfig before would those want to be removed also since they would fall under the same redundancy?

@RDIL RDIL added the major label Sep 8, 2022
export the command from `@yarnpkg/shell`
Typescript can find the type definition files without these helpers.
@Larry1123 Larry1123 requested a review from merceyz September 8, 2022 19:58
@arcanis
Copy link
Member

arcanis commented Sep 15, 2022

I think we can go with the removes types field, and publish a rc - if there's a problem it should hopefully be spotted there

@arcanis arcanis merged commit 4d0eb09 into yarnpkg:master Sep 15, 2022
@@ -3,7 +3,7 @@ import {Configuration, StreamReport} from '@yarnpkg/core';
import {Command, Option, Usage} from 'clipanion';

// eslint-disable-next-line arca/no-default-export
export default class PluginListCommand extends BaseCommand {
export default class PluginRuntimeCommand extends BaseCommand {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for cleaning all this, btw!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants