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

refactor: move exports.types to top #29

Merged
merged 1 commit into from Jul 15, 2022
Merged

refactor: move exports.types to top #29

merged 1 commit into from Jul 15, 2022

Conversation

@privatenumber
Copy link
Owner

I've seen that but TS seemed to have no problem reading types for me. Not to mention, object keys inherently have no order.

Is it actually breaking something for you?

@JounQin
Copy link
Contributor Author

JounQin commented Jul 5, 2022

It is required when using moduleResolution: "Node16" or moduleResolution: "NodeNext".

image

Please read the reference I linked.

The order in exports matters.

See also in Node's official document

@JounQin
Copy link
Contributor Author

JounQin commented Jul 5, 2022

7d501be

I noticed you actually added correct types for imports for eslint-import-resolver-typescript

@privatenumber
Copy link
Owner

Please read the reference I linked.

I've read it already and I even followed up here.

I noticed you actually added correct types for imports for eslint-import-resolver-typescript

I don't know what you're talking about.

Anyway, is it actually breaking something for you?

(Also, please use the Edit Comment feature instead of commenting multiple times in a row.)

@JounQin
Copy link
Contributor Author

JounQin commented Jul 5, 2022

I've read it already and I even microsoft/TypeScript-Website#2120 (comment).

See: It is required when using moduleResolution: "Node16" or moduleResolution: "NodeNext", it will be broken when enabled.

See also import-js/eslint-import-resolver-typescript#113

I don't know what you're talking about.

I referenced your previous commit 7d501be

@privatenumber
Copy link
Owner

I'm wondering if you're just saying that because the docs say so, or if you're actually experiencing not getting types in a project.

Please answer if it's actually breaking something for you.

@JounQin
Copy link
Contributor Author

JounQin commented Jul 5, 2022

if you're actually experiencing not getting types in a project.

Yes, I start to use moduleResulution: "Node16" in recent days, incorrect types breaks ts.

@privatenumber
Copy link
Owner

I'm interested in investigating that.

I tried that out and it works fine:
Screen Shot 2022-07-05 at 2 22 33 PM

Can you provide reproduction details?

@privatenumber
Copy link
Owner

Any updates @JounQin ?

@JounQin
Copy link
Contributor Author

JounQin commented Jul 14, 2022

Any updates @JounQin ?

I didn't spend much time on it, it's just spec.

@privatenumber
Copy link
Owner

If you don't plan to investigate it, I will merge it as a refactor because fix triggers a release.

I hope you do though because you said the types weren't working for you.

@JounQin
Copy link
Contributor Author

JounQin commented Jul 15, 2022

If you don't plan to investigate it, I will merge it as a refactor because fix triggers a release.

I hope you do though because you said the types weren't working for you.

That's fine, if I meet the error again, I'll post here.

Personally, I think a change align to spec should trigger a release.

Whatever, it doesn't matter.

@privatenumber
Copy link
Owner

Prefer not to version bump if we don't know what the impact of the change is.

@privatenumber privatenumber changed the title fix: types field should always come first refactor: move exports.types to top Jul 15, 2022
@privatenumber privatenumber merged commit adedf57 into privatenumber:develop Jul 15, 2022
@JounQin JounQin deleted the patch-1 branch July 15, 2022 01:00
@JounQin
Copy link
Contributor Author

JounQin commented Aug 5, 2022

Prefer not to version bump if we don't know what the impact of the change is.

@privatenumber

I think you're right here now.

Like microsoft/TypeScript#50067 and unplugin/unplugin-auto-import#243, I think we should prefer type: "module".

HDYT?

@privatenumber
Copy link
Owner

privatenumber commented Aug 5, 2022

Thanks for the insightful reference. I'll keep this in mind in future projects.

I'm currently don't see a problem in get-tsconfig though. Maybe it's the named export. It's working here with moduleResolution: "node16": https://stackblitz.com/edit/node-5pfnos?file=index.ts

@JounQin
Copy link
Contributor Author

JounQin commented Aug 5, 2022

I'm currently don't see a problem in get-tsconfig though. Maybe it's the named export. It's working here with moduleResolution: "node16"

That's great then! I'm also always preferring named export everywhere.

@privatenumber
Copy link
Owner

privatenumber commented Dec 22, 2022

I updated the exports as per the recommendation of the TypeScript Issue you linked:
8516d5a

@privatenumber
Copy link
Owner

🎉 This PR is included in version 4.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

None yet

3 participants