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

build: leave code as-is #560

Merged
merged 29 commits into from
Feb 23, 2024
Merged

Conversation

himself65
Copy link
Member

@himself65 himself65 commented Feb 22, 2024

  • Remove bundler, just compile code to cjs/esm
  • Switch to ESM by default to support modern ES features like tree shake, subpath import, and strict ts check
  • Use vitest

We don't bundle the code since it puts lots of effort, and I think bundling is the downstream issue (like next.js bundler or vite)

And we keep the file as is so that everyone can import it directly

Followup: #560 (comment)


Thanks for some TypeScript QA from @Jack-Works

Copy link

changeset-bot bot commented Feb 22, 2024

⚠️ No Changeset found

Latest commit: 14e8d58

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Feb 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
llama-index-ts-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 23, 2024 11:36pm

Copy link
Member Author

@himself65 himself65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not merge now. Still have issue with ESM

Copy link
Collaborator

@marcusschiesser marcusschiesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! Mostly asking questions as we're introducing a lot of new tools with this PR (so we should be aware of the risks).

examples/ollama.ts Show resolved Hide resolved
packages/core/scripts/modify-package-json.mjs Outdated Show resolved Hide resolved
packages/core/package.json Outdated Show resolved Hide resolved
packages/core/scripts/modify-package-json.mjs Outdated Show resolved Hide resolved
packages/core/src/PromptHelper.ts Show resolved Hide resolved
packages/core/package.json Show resolved Hide resolved
packages/core/package.json Show resolved Hide resolved
packages/core/package.json Show resolved Hide resolved
packages/core/package.json Outdated Show resolved Hide resolved
packages/core/scripts/modify-package-json.mjs Outdated Show resolved Hide resolved
@himself65
Copy link
Member Author

There are some possibilities for us to publish this package: first is bundle vs no-bundle, and second is esm vs cjs.

Bundle vs No-Bundle

The issue with bundle llamaindex is it's too large, and if we bundle the whole llamaindex into one js file, that will cause many downstream problems.

For example, in next.js, you have to set externals or server side packages externals to ensure next.js(webpack) would not bundle the code.
WHY? because we bundle them into one file, everything inside that file will be treated as an important module and cannot be tree-shaked.

// ./dist/index.js
import 'pg'
import 'mongo'
import 'openai'
import 'xxx'
// ...
export ...

You can see tons of packages are included at the top of the entry; that's why.

At the very first, I tried to use bunchee to bundle the code and use conditional exports to separate the code into different files. However I realized that it was to waste of time/memory and it actually spent more time on solving the bundle issue.

So from to beginning issue, we just want the user could control import so that it's possible for downstream to manually tree-shake and still not waste our members' too much time on this.

ESM vs CJS

First, llamaindex should be an ESM package, not because ESM is the future or something. More reasons are because the current TypeScript typecheck or modern bundler is more strict on ESM modules instead of CJS. It will be much easier for the analysis tool to check our code. For example, in this PR, I change the module to node16 and moduleResolution to node16. WHY? Because it will require you to write file extension .js as you can see. And it's the suggested way by TS and modern JS code style. (See discussion on microsoft/TypeScript#42813 (comment))

Also, change to ESM could help llamaindex support multiple environments like (Deno, Edge runtime, Bun, or even browsers) because ESM is the standard nowadays

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