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 where needed
#2327
base: main
Are you sure you want to change the base?
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Since this PR is approved - is there a chance that it could land? Or do you have some specific time at which you land approved PRs? |
Personally I prefer to keep the main branch in line with the published release, and don't want to release too often. Realistically, there isn't a rush to land this. It isn't causing known problems and is on a little used entry point. (In the meantime, you could update the PR to current |
The checking website mentioned in the linked thread actually complains about almost everything for I think I need to do some rereading about how |
So this PR's focus is on fixing the "🐛 Used fallback condition" - since that's related to a bug in TypeScript itself. Everything else is out of the scope of this PR. However, I can advise how to properly fix just any of those problems - or even do the work myself if needed (but for that I would have to know more about the project itself).
If your types mainly live in DefinitelyTyped then ye - I would expect that "❌ No types" is OK here since
Yes, it's quite complicated - especially if you are trying to ship a "dual package" (and you do). If you have 2 separate implementation files (for require and import conditions) then it's incorrect to assume that you can only use a dingle set of declaration files (even though that's what most assume). The runtime code and the types should match with each other 1 to 1 - if you have 2 different runtime codes you also need 2 separate declaration files (if you want to have 100% correct setup etc). |
Just checking my understanding. I think another way of handling this would be to delete the (Not requesting a PR change. I am happy listing it.) |
In this specific situation, it wouldn't be a good change. You can rely on what I call "a sibling lookup" but your extensions should line up. So the problem, here, is that your implementation file is That's likely a problem that will be caught by I totally get that this whole thing is quite confusing and not that well-documented. It's rather easy to end up with some subtle problems. If you want to get this completely right, it might be best to schedule a call or something to guide you through this. |
Thanks for the comments. I missed the inconsistent file extension, good point! I thought that might be ok because of the Keep it simple, just reorder for now... |
Oh yea, I missed that! In this case - yeee, it should be somewhat OK but it becomes harder to reason about and stuff. So my recommendation is to always keep matching extensions. |
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.