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

Don't require setting moduleResolution to bundler with this one weird trick #1238

Open
ericallam opened this issue Sep 20, 2024 · 9 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@ericallam
Copy link

Your note about setting moduleResolution to "bundler" in tsconfig.json made me curious (this shouldn't be necessary in a dual-package setup), so I just checked your types and it looks like there are a good many issues:

CleanShot 2024-09-20 at 09 12 26

https://arethetypeswrong.github.io/?p=llamaindex%400.6.4

I highly recommend looking into building with tshy, it makes dual-package really simple. We recently moved to it in trigger.dev and now our types look like this:

CleanShot 2024-09-20 at 09 14 57

@marcusschiesser
Copy link
Collaborator

@himself65 that looks cool - we can also run https://www.npmjs.com/package/@arethetypeswrong/cli in our CI

@himself65 himself65 added the good first issue Good for newcomers label Sep 20, 2024
@himself65
Copy link
Member

That's amazing, let me check

@himself65 himself65 added the bug Something isn't working label Sep 20, 2024
@himself65
Copy link
Member

himself65 commented Sep 20, 2024

Good new is that our core module is mostly correct

https://arethetypeswrong.github.io/?p=%40llamaindex%2Fcore%400.2.4

@himself65
Copy link
Member

I think this is because we didn't use bundler in llamaindex module for some reasons. Maybe I should bring it back

@himself65 himself65 added enhancement New feature or request help wanted Extra attention is needed and removed bug Something isn't working labels Sep 20, 2024
@himself65
Copy link
Member

I know the solution in the repo, but I think it's costly now because I don't want put many effort on bundle stuff.

each bundler has advantages but also disadvantage, for example tshy will help dual package hazards, but we need exports different module for different js environment, that's why I choose bunchee (comparing with tsup, it's more simple to use).

So, Ideally we should write our custom bundle process using rollup, something combiniation similar to jotai and react.

  1. different JS environment export conditions based on file extension .node.js, .edge-light.js, or use gate way in comment
  2. compatabile with all tsconfig.json (as this issue)

For now I don't have time to make this happen, but I will add to roadmap of stable release.

@ericallam
Copy link
Author

tshy does support the adding export conditions for different file paths, but if you want to use something other than tsc for building that is not currently supported.

@himself65
Copy link
Member

Working on this right now

@himself65
Copy link
Member

himself65 commented Sep 24, 2024

blocked by huozhi/bunchee#579

Just releazed that there is a bug on bunchee 😢

@himself65
Copy link
Member

OK, core module is supporting node10 now. I cannot bundle llamaindex package because there're too many thrid party npm issue

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

3 participants