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

chore: use type PackageJson from type-fest #117

Closed
wants to merge 2 commits into from

Conversation

shulaoda
Copy link

Summary

The preparatory work for PR #88 is to avoid endless resolution of branch conflicts.

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

@shulaoda
Copy link
Author

@Timeless0911 cc

@Timeless0911
Copy link
Collaborator

I think there is no need to import this dependency. Is there any use?

@shulaoda
Copy link
Author

I think there is no need to import this dependency. Is there any use?

It mainly utilizes the type PackageJson, which we will use in PR #88. Perhaps we should simply detach it and embed it inside?

@Timeless0911
Copy link
Collaborator

Introducing additional packages will cause that if we provide a public js API in the future, its type will not be bundled since we use bundleless DTS. We need to avoid relying on this type in our public API, this creates an extra mental burden.

@shulaoda shulaoda marked this pull request as draft August 21, 2024 12:13
@fi3ework
Copy link
Member

We want Rslib's dependencies to be as minimal as possible, and type-fest is only used for type imports here, so we don't want it to be included in dep.

Ideally, we should be able to specify type-fest as a devDep without mental burden, and then bundle in the bundled dts. However, currently, there's no battle-tested dts bundle method available, so Rslib still uses bundleless types for now, and we might switch to bundle in the future.

Copying the usable code (like https://github.com/sindresorhus/type-fest/blob/main/source/package-json.d.ts#L221-L225) seems to be the best solution for now.

@shulaoda shulaoda closed this Aug 23, 2024
@shulaoda shulaoda deleted the chore/type-fest branch August 23, 2024 08:05
@shulaoda shulaoda restored the chore/type-fest branch August 23, 2024 08:12
@shulaoda shulaoda reopened this Aug 23, 2024
@shulaoda shulaoda marked this pull request as ready for review August 23, 2024 08:45
@shulaoda
Copy link
Author

Is it okay to do this?

@fi3ework
Copy link
Member

Overall it looks good, because we have copied the d.ts code, so we can do some further optimization here

  1. Remove unnecessary fields and keep only the fields that may be needed now and in the future to reduce the size of d.ts (exports / main / module / types / publishConfig, there may be others that I haven't thought of yet, we can continue to copy from type-fest later).
  2. Remove the legacy writing method of declare namespace and adopt the ESM writing method.

@shulaoda
Copy link
Author

Remove unnecessary fields and keep only the fields that may be needed now and in the future to reduce the size of d.ts.

I will complete the rest in PR #88

@shulaoda shulaoda closed this Aug 24, 2024
@shulaoda shulaoda deleted the chore/type-fest branch August 24, 2024 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants