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

Support NextJS edge runtime #618

Merged
merged 7 commits into from
Mar 18, 2024
Merged

Support NextJS edge runtime #618

merged 7 commits into from
Mar 18, 2024

Conversation

marcusschiesser
Copy link
Collaborator

@marcusschiesser marcusschiesser commented Mar 7, 2024

This PR is working with NextJS edge run-time (tested with Chat LlamaIndex (using pnpm build && pnpm start - pnpm dev doesn't work yet))

Changes:

  1. Make core edge compatible
  2. Removed non-edge compatible deps

Next steps:
Discuss how to deal with non-edge compatible deps - see comments

Copy link

changeset-bot bot commented Mar 7, 2024

🦋 Changeset detected

Latest commit: afc000b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
llamaindex Patch
@llamaindex/experimental Patch

Not sure what this means? Click here to learn what changesets are.

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

Copy link

vercel bot commented Mar 7, 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 Mar 18, 2024 8:06am

Copy link
Member

@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.

My suggestion would be change "index.edge-ligtht.js" and only export core module.

But no matter how, this would need more effort because need to change many path import

@himself65
Copy link
Member

But no matter how, this would need more effort because we need to change many path import

For example, some files would indirectly import vectorStore.js

index.edge-light.js -> ... -> verctorStore.js

"exports": {
"./readers/SimpleDirectoryReader": {
"import": {
"types": "./dist/type/readers/SimpleDirectoryReader.edge.d.ts",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could call this EdgeDirectoryReader then we won't need this edge specific export

"license": "MIT",
"type": "module",
"dependencies": {
"@anthropic-ai/sdk": "^0.15.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

best we generate this file from core/package.json and copy its dependencies and version

export * from "./objects/index.js";
export * from "./postprocessors/index.js";
export * from "./prompts/index.js";
export * from "./index.edge.js";
export * from "./readers/index.js";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

here's the trick: edge is neither importing readers nor storage

Copy link
Member

@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.

seems not working

@@ -0,0 +1,80 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Is this package empty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, idea is to just copy the build from core and then use different exports in this package

@@ -0,0 +1,31 @@
export * from "./ChatHistory.js";
Copy link
Member

Choose a reason for hiding this comment

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

This seems to work, but might need test to make sure won't have regression

@himself65
Copy link
Member

seems not working

working, but some deps seem not correct

"lint": "next lint"
},
"dependencies": {
"llamaindex": "workspace:*",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should use @llamaindex/edge package

@@ -60,14 +60,12 @@
".": {
"import": {
"types": "./dist/type/index.d.ts",
"edge-light": "./dist/index.edge.js",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@himself65 the idea is that @llamaindex/edge is working with edge, but llamaindex is not - that's why i removed edge-light from the core package

@marcusschiesser marcusschiesser marked this pull request as ready for review March 18, 2024 08:13
@marcusschiesser marcusschiesser merged commit 259c842 into main Mar 18, 2024
13 checks passed
@marcusschiesser marcusschiesser deleted the ms/use-edge-run-time branch March 18, 2024 08:13
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