Skip to content

rename kv default export#131

Merged
correttojs merged 6 commits intomainfrom
fabiobenedetti/rename-default-kv-export
May 11, 2023
Merged

rename kv default export#131
correttojs merged 6 commits intomainfrom
fabiobenedetti/rename-default-kv-export

Conversation

@correttojs
Copy link
Collaborator

@correttojs correttojs commented May 9, 2023

move kv default export to a named export

Closes #107.

@changeset-bot
Copy link

changeset-bot bot commented May 9, 2023

🦋 Changeset detected

Latest commit: 7923689

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

This PR includes changesets to release 1 package
Name Type
@vercel/kv Minor

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

@vercel
Copy link
Contributor

vercel bot commented May 9, 2023

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

Name Status Preview Comments Updated (UTC)
storage ❌ Failed (Inspect) May 11, 2023 7:25am
vercel-storage-next-integration-test-suite ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 11, 2023 7:25am

"dev": "next dev",
"integration-test": "playwright test",
"lint": "next lint",
"lint": "eslint --max-warnings=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.

next lint caused a lint-staging failure

Comment on lines +88 to +92
get() {
throw new Error(
'"The default export has been moved to a named export, change to import { kv }"',
);
},

Choose a reason for hiding this comment

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

Suggested change
get() {
throw new Error(
'"The default export has been moved to a named export, change to import { kv }"',
);
},
get(target, prop, receiver) {
if (prop === 'then' || prop === 'parse') return Reflect.get(target, prop, receiver);
throw new Error(
'"The default export has been moved to a named export, change to import { kv }"',
);
},

Since this is going to be removed for V1, we could do this to prevent it from always throwing in Vite. It's the disgusting hacky solution we didn't want to implement for the default export, but since it'll go away at 1.0, I'd be comfortable with it.

Choose a reason for hiding this comment

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

Not positive this exact thing would work (would have to test with Vite), but something like it should.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why not add a console.warn to the default export, saying that

'"The default export will be moved to a named export after the beta, change to import { kv }"',

without throwing any error?

This way the npm version is a real minor (no breaking changes)

To sum up:

  • 0.2.0 adds the named export, fixes Vite using the hacky solution, warns every user about the upcoming breaking change
  • 1.0.0 removes the default export as soon as we go GA

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was able to reproduce the Vite issue in a jest test, I added a test case.
I changed the throw to a colored console.warn, but indeed it may not be that visible

@elliott-with-the-longest-name-on-github
Copy link
Collaborator

Ahhh nice, this is perfect. No breaking change until 1, a warning, and the nasty hacky fix on top. :chef-kiss:

@elliott-with-the-longest-name-on-github
Copy link
Collaborator

Looks like unit tests are failing though 😬

@correttojs
Copy link
Collaborator Author

Looks like unit tests are failing though 😬

oh yeah, sorry I forgot to review them!

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.

KV: Problem importing in local mode with SvelteKit

2 participants