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: move types condition to the front #26630

Merged
merged 7 commits into from
May 11, 2023

Conversation

Andarist
Copy link
Contributor

I moved types condition to the front. package.json#exports are order-sensitive - they are always matched from the top to the bottom. When a match is found then it should be used and no further matching should occur.

Right now, the current setup works in TypeScript but it's considered a bug and it should not be relied upon, see the thread and the comment here. For that reason, I would like to fix all popular packages that misconfigured their exports this way so the bug can be fixed in TypeScript.

⚠️ note that this only fixes one problem (🐛 Used fallback condition) but this package still won't be completely correct: arethetypeswrong.

@warrensplayer warrensplayer requested review from lmiller1990 and a team and removed request for a team May 3, 2023 19:39
Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Did not know about this ordering. I wonder if we can link for this.

Can you add a CHANGELOG entry? https://github.com/cypress-io/cypress/actions/runs/4840113756/jobs/8625561864?pr=26630#step:4:18. This should guide you - we lint the changelog, too.

@Andarist
Copy link
Contributor Author

Andarist commented May 9, 2023

Did not know about this ordering. I wonder if we can link for this.

This is mentioned here:

Within the "exports" object, key order is significant. During condition matching, earlier entries have higher priority and take precedence over later entries. The general rule is that conditions should be from most specific to least specific in object order.


Can you add a CHANGELOG entry? https://github.com/cypress-io/cypress/actions/runs/4840113756/jobs/8625561864?pr=26630#step:4:18. This should guide you - we lint the changelog, too.

Done.

@jordanpowell88 jordanpowell88 changed the title fix: move types condition to the front fix: move types condition to the front May 10, 2023
@jordanpowell88
Copy link
Collaborator

@Andarist Can you update the commit body to match our Issue template? It's causing the Semantic Pull Request step in CI to fail

@Andarist
Copy link
Contributor Author

@jordanpowell88 I think i've corrected this as needed right now.

Offtopic... I'm Changesets maintainer and it seems that this fairly popular project could help you to manage CHANGELOG generation and stuff like that. Might not be a perfect fit right now if you have custom needs but a lot can probably be scripted/automated around it and you could open feature requests for missing things. You probably run into a lot of such merge conflicts and stuff when editing CHANGELOG.md manually on different branches so I would expect that a tool like this would be quite useful for your team.

@jordanpowell88
Copy link
Collaborator

Thanks @Andarist! Yeah we currently manage our CHANGELOG manually but we will consider this if we reevaluate the way we handle our releases in the future.

@lmiller1990
Copy link
Contributor

Seeing some unrelated flake, going to try fix it here: 9d6ba66

@lmiller1990
Copy link
Contributor

Still hitting CI weirdness... debugging. I am sure it's unrelated to the changes here. I don't know why it's flaking.

@lmiller1990 lmiller1990 merged commit 3335e4b into cypress-io:develop May 11, 2023
3 checks passed
@MikeMcC399
Copy link
Contributor

executing yarn reverts the order in cli/package.json leaving a git diff

for example:

     "./vue": {
-      "types": "./vue/dist/index.d.ts",
       "import": "./vue/dist/cypress-vue.esm-bundler.js",
-      "require": "./vue/dist/cypress-vue.cjs.js"
+      "require": "./vue/dist/cypress-vue.cjs.js",
+      "types": "./vue/dist/index.d.ts"
     },

@Andarist
Copy link
Contributor Author

It seems that you depend on https://github.com/keithamus/sort-package-json through https://github.com/kuceb/eslint-plugin-json-format . I can file an issue there and maybe even fix this but it might take a moment. What about disabling this ESLint rule temporarily?

@Andarist
Copy link
Contributor Author

Ok, it turned out that actually your own scripts were at "fault" here so I managed to fix it without any delays: #26743

@MikeMcC399
Copy link
Contributor

@Andarist

@cypress-bot
Copy link
Contributor

cypress-bot bot commented May 23, 2023

Released in 12.13.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v12.13.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators May 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants