fix(sort-collections): avoid sorting exports by default
#918
Conversation
This causes problems because the order of export conditions is critical to how the package will be interpreted.
Example, the following will be improperly sorted
```json
{
"types": "./f.d.ts",
"import": "./f.js",
"default": "./f.cjs"
}
```
Producing the following invalid config
```json
{
"default": "./f.cjs"
"import": "./f.js",
"types": "./f.d.ts",
}
```
Issues:
1. "default" is at the top. "default" should always be last.
2. "types" is after "import". This is invalid because typescript will use the first matching condition, "import" being a valid condition used by typescript.
|
The original PR that introduced this assumed a over-simplified scenario. For a more thorough approach for sorting the exports field, see this PR in |
GeorgeTaveras1231
changed the title
exports from defaultexports from default
GeorgeTaveras1231
changed the title
exports from defaultexports from default
GeorgeTaveras1231
changed the title
exports from defaultexports by default
There was a problem hiding this comment.
👍 SGTM. Ideally we like to discuss fixes in issues first, but this one seems straightforward to me (cc @michaelfaith to make sure I haven't missed something obvious).
Just, could you add unit testing please? Since this touches program logic it should also have changes in sort-collections.test.ts. Thanks!
JoshuaKGoldberg
added
the
status: waiting for author
+1 on the unit tests. @GeorgeTaveras1231 I guess i'd also ask how this relates to the upstream change you have in flight. As in, once that lands and is released, should this be re-enabled? |
|
@JoshuaKGoldberg @michaelfaith thanks for the comment. I apologize for the carelessly low-touch effort on this PR - I was lazy and had multiple ongoing threads so decided to use the GH code editor and make the change in-line 😅 -- no worries, I will pull down the repo and update the unit tests 👍🏽 @michaelfaith this relates loosely with my other PR because this rule does not use the I think the |
|
Thanks. And I just noticed the defaults are listed on the doc page for the rule too, so that'll need to be updated as well. 🙏 |
.tool-versions
Outdated
There was a problem hiding this comment.
I hope it's not a problem that I committed this. I use asdf for managing tool versions and it's helpful to have these files in the repo. I thought it would help other devs who might use the same tool.
There was a problem hiding this comment.
I like the idea in theory, but in practice:
.nvmrcis already here. This adds another, duplicate place where engine versions are declared.- I'm not even sure I want to keep that one long-term in the template this repo is based on: 🚀 Feature: Use pnpm as a Node.js environment manager create-typescript-app#805 (see also: 🚀 Feature: Add packageManager back to the root
package.jsoncreate-typescript-app#1906)
- I'm not even sure I want to keep that one long-term in the template this repo is based on: 🚀 Feature: Use pnpm as a Node.js environment manager create-typescript-app#805 (see also: 🚀 Feature: Add packageManager back to the root
- I've been trying to reduce the number of root-level config files. IMO the more exist in a repo, the more intimidating that repo is to new folks. We already have a lot of stuff!
Could you please split this out of this PR? I'm not opposed to discussing in a separate issue -here or in CTA- but would rather each issue and PR be minimally focused.
|
@michaelfaith @JoshuaKGoldberg I have a question. I put a lot of effort into researching and figuring out a robust algorithm for sorting the conditions in TBH, I think I'm having some difficulty communicating with the maintainers of that repo; but I get the sense that they don't have a real position on the validity of the algorithm and are just resistant to the change because they don't understand it. (I may be looking to into it). My question is: Would you be open to me incorporating that algorithm to this ESLint plugin directly? (without using the I work in a large monorepo with hundreds of packages and see a huge benefit to enforcing a predictable order - both in my repo and the open source community -- not only for stylistic reasons, but also to ensure developers avoid unexpected bugs while trying to support multiple runtimes, environments, and syntaxes. Feel free to look at the other PR if you are curious about the approach - I list a lot of examples showing invalid and valid configurations that the sorter will enforce, as well as links to the resources I used in my research. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #918 +/- ##
==========================================
- Coverage 98.77% 98.77% -0.01%
==========================================
Files 21 21
Lines 1222 1221 -1
Branches 142 142
==========================================
- Hits 1207 1206 -1
Misses 15 15 ☔ View full report in Codecov by Sentry. |
This is a tricky topic to respond to in public 😄. Giving feedback on anything around communication styles, ongoing debates, or other opinion-oriented things can easily look like personal attacks. Please forgive me if anything here looks like that. It's not meant to be criticism, just general advice. (I don't think you'll take anything as pointed, but have in the past seen people misread others' discussions...) Anyway, this is just my read of this PR and that one:
Happy to chat more over email on navigating the murky waters of open source politicking. 🙂 |
| @@ -132,6 +132,7 @@ ruleTester.run("sort-collections", rule, { | |||
| } | |||
| } | |||
| }`, | |||
| options: [["exports"]], | |||
There was a problem hiding this comment.
[Testing] Oh, sorry, I wasn't clear in what I meant to ask for - could you please add a valid test too?
I think there are really two points of logic that are relevant+important to test for this change:
- By default, the rule does not enforce sorting for
"exports" - If added to the list in
options,"exports"does get sorted
This test checks the latter but not the former.
There was a problem hiding this comment.
@JoshuaKGoldberg about this - should we consider warning developers or requiring a separate flag to enable dangerous keys? I think there is an opportunity here to educate people on the dangers of sorting exports and imports alphabetically (while still giving them the choice if this is what they want to do).
There was a problem hiding this comment.
Maybe as a quick mention in the docs file that links to the first-party package.json docs? I don't think a separate flag is justified yet. Someone who reads through the docs to know about the option is probably pretty high intent anyway.
There was a problem hiding this comment.
Hahaha that is fair
I'm laughing because I'm realizing I have a cynical tendency to think people will not read the documentation lol
No worries, I will add a note in the docs 👍🏽
There was a problem hiding this comment.
@JoshuaKGoldberg I think this is my last alignment question.
I was working on the docs and realized that this rule only does a top-level sort (not recursively) so at the moment, it only causes problems if you have a condition object at the top level.
i.e.
This breaks when auto-fixed
{
"exports": {
"types": "./f.d.ts",
"import": "./f.js",
"default": "./f.cjs"
}
}This is ok
{
"exports": {
".": {
"types": "./f.d.ts",
"import": "./f.js",
"default": "./f.cjs"
}
}
}Since the error case is easily distinguishable, it made me think we are putting too much responsibility on the consumer to know wether its safe or not safe to sort.
What do you think about just adding a check to the sorter? - to only sort if its not a condition object (the heuristic per the standards is to check if the keys start with .)
It would be a totally different approach, we may be able to leave exports in the list of defaults but make it safer.
I'm ok with putting this in a separate PR and closing this one, or repurposing this one. What do you think?
|
@JoshuaKGoldberg, thanks so much for the thoughtful response, constructive feedback, and offer to connect outside of GH. I am grateful that you'd put so much thought into your response when I'm a stranger on the internet lol and in retrospect, I was being a baby about the whole thing 😅 In my defense, I think what I was looking for that I was not getting was validation that this is a real problem and that the approach is sound (hence my comment about them "not understanding it"). I'm totally happy to break up PRs if the scope feels too big - but I don't think that was ever requested in those terms 🤔 I just heard "delete all this unnecessary code" 😅 🤷🏽 You're still making a lot of good points and giving me great perspective. 🙏🏽 And I'm sorry to make a GH PR thread into a personal therapy session. I think I need an outlet for these long tangential conversations so I may take you up on that offer to connect via email 😌 |
|
If you're thinking of implementing a new sort algorithm rather than removing exports from the default sort, i'd recommend closing this PR out and starting with an issue. We can hash out details about behavior in the issue, and after there's general alignment, then open a PR against the issue. How does that sound? |
|
@michaelfaith That sounds good! I'll go ahead and open an issue proposing the algorithm I worked on - we can use that as a baseline and adjust it after gathering feedback. |
|
@michaelfaith FYI I opened #928 with a proposal. It's a bit lengthy and I apologize for that but take the time to read it if you are interested. This was a good exercise because as I was writing it I discovered issues with my original approach - so I will likely close the PR in |
This causes problems because the order of export conditions is critical to how the package will be interpreted.
Example, the following will be improperly sorted
{ "types": "./f.d.ts", "import": "./f.js", "default": "./f.cjs" }Producing the following invalid config
{ "default": "./f.cjs" "import": "./f.js", "types": "./f.d.ts", }Issues:
PR Checklist
status: accepting prsOverview