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!: subpackage exports types #111

Merged
merged 3 commits into from
Feb 12, 2025
Merged

Conversation

userquin
Copy link
Contributor

@userquin userquin commented Dec 18, 2024

This PR includes:

  • fixes subpackages exports, right now only proper types for bundler module resolutions (check screenshots below)
  • adss typesVersions to resolve types for subpackes exports other than default for node10 module resolution
  • includes .idea in .gitignore to exclude JetBrains IDE stuff (similar to .vscode folder)
  • adds files to package.json to include only dist folder, we don't need 800KB of source code, you've the repo here (from +800KB to 260KB only)
  • adds scripts/postbuild.mjs to add .js extension in dts files importing from dist folder and runs it in the build script

We can remove also the require entry in the default package exports since the build doesn't generate cjs files, the module using imports. Why do we need the ./dist/react-legacy/pwa-install.react-legacy.js in the package exports? This should be removed , importing from @khmyznikov/pwa-install/react-legacy should now work.

NOTE: you should check it before releasing it in some project to check if the changes in this PR works (I can check it using Vite in this repo https://github.com/sunflower-land/sunflower-land/blob/e42df72f13b17fd514e8953855c858ca79b464f4/src/features/pwa/PWAInstallProvider.tsx#L10).

Without this PR (https://arethetypeswrong.github.io/?p=%40khmyznikov%2Fpwa-install%400.4.8):

imagen

With this PR (run npm run build && npm pack in this PR and then upload the tgz file to https://arethetypeswrong.github.io/):

imagen

@userquin
Copy link
Contributor Author

Working using Vite: check sunflower-land/sunflower-land#4987 (expand Install working on my local on my android phone (via usb))

@khmyznikov khmyznikov merged commit 72d7b53 into khmyznikov:main Feb 12, 2025
@userquin
Copy link
Contributor Author

@khmyznikov we should review the implementation resolution for node10, looks like using d.ts in the typesVersions entry resolving to TypeScript (it should resolve to the Javascript .js): expand details and for each entrypoint expand node10 and check implementation (check screenshots here shikijs/shiki#922 (comment))

@userquin userquin deleted the fix-types branch February 12, 2025 14:57
@userquin
Copy link
Contributor Author

userquin commented Feb 12, 2025

The fix should be easy, just use .ts or .mts extension inside typesVersions (we need to check it here)

@khmyznikov
Copy link
Owner

@userquin is it just for node10? I don't mind to just leave it as is.

@userquin
Copy link
Contributor Author

userquin commented Feb 12, 2025

yes, it is only about node10 module resolution (you can check the entrypoints for node16 and bundler, should be fine resolving to .js with TypeScript false)

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.

2 participants