search

astoilkov / use-local-storage-state

React hook that persists data in localStorage
MIT License
1.09k stars 39 forks source link

fix(compat): fix compatibility for vitest #59

Closed smeng9 closed 1 year ago

smeng9 commented 1 year ago

Fix exports field for vitest so it can be correctly resolved in test environment

astoilkov commented 1 year ago

Thanks for the contribution!

I don't understand much about the new exports field. This is why I have a few questions (if you know the answers).

By the description, I'm assuming that testing with Vitest code that includes the use-local-storage-state library throws an error. Am I right and what's the error?

Isn't the below better (having "require" point to the CJS version)? What's the difference? I saw this in the Node.js docs — https://nodejs.org/api/packages.html#conditional-exports.

"exports": {
    "import": "./es/index.js",
    "require": "./index.js"
}

Is this breaking? Meaning should I release a new major or minor version?

smeng9 commented 1 year ago

You are correct, vitest throws an error. The error is vitest does not use module field or jsnext:main field in the package.json Error: Failed to resolve entry for package "tg-use-local-storage-state". The package may have incorrect main/module/exports specified in its package.json.

Oh I just noticed there is indeed a cjs copy in the final published package. Your solution would be better. I should use "exports": { "import": "./es/index.js", "require": "./index.js" }

This should not be breaking. It merely improves the compatibility with vitest.

astoilkov commented 1 year ago

Awesome. Thanks for your contribution!

astoilkov commented 1 year ago

This PR introduced this bug: https://github.com/astoilkov/use-local-storage-state/issues/60.

@smeng9 Any ideas why?

astoilkov commented 1 year ago

I reverted this fix because it breaks type definitions.