Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update exports for @zarrita/ndarray #130

Merged
merged 1 commit into from
Jan 14, 2024
Merged

Conversation

katamartin
Copy link
Collaborator

👋 I've been (finally!) playing around with integrating @carbonplan/maps with zarrita. It's been a pretty smooth transition, except when I have a basic import from @zarrita/ndarray

import { get } from '@zarrita/ndarray'

I see the following error

Module not found: Package path . is not exported from package
/Users/katamartin/carbonplan/maps/node_modules/@zarrita/ndarray (see exports field in 
/Users/katamartin/carbonplan/maps/node_modules/@zarrita/ndarray/package.json)

I think this should be resolved by updating the exports for@zarrita/ndarray to point to the correct index files (and match the @zarrita/storage exports!). I haven't actually tested this, so I very well may be missing something here!

@manzt
Copy link
Owner

manzt commented Jan 14, 2024

Thanks for making this fix, although I think this will make sure the imports/types work within the zarrita repo but shouldn't affect what is published to NPM. We use publishConfig to override exports when publishing to npm (more info)

	"publishConfig": {
		"main": "dist/src/index.js",
		"exports": {
			".": {
				"types": "./dist/src/index.d.ts",
				"import": "./dist/src/index.js"
			}
		}
	}

Are you installing from zarrita@next?

@manzt manzt merged commit 480dc2c into main Jan 14, 2024
2 checks passed
@manzt manzt deleted the katamartin/ndarray-exports branch January 14, 2024 16:28
@katamartin
Copy link
Collaborator Author

Oooh interesting, wonder what I'm hitting. Yeah I installed zarrita@next and am specifically using zarrita@^0.4.0-next.6 and @zarrita/ndarray@^0.1.0-next.6

@manzt
Copy link
Owner

manzt commented Jan 16, 2024

I'll try a fresh import myself and let you know what I figure out...

@manzt
Copy link
Owner

manzt commented Jan 17, 2024

Ok, took a look into this. The issue is that the package built with microbundle from @carbonplan/maps generates a CommonJS package (module system used in Node) and a modern ESM package (module system used in browsers).

Zarrita only ships ESM, and Next.js resolve the import for @carbonplan/maps to main: "dst/index.js", which is CJS and fails due to require-ing ESM:

const zarr = require("zarrita");

I was able to get the demo working in @carbonplan/maps but updating the next config to prefer the module export in package.json over main:

error: cannot run delta: No such file or directory
diff --git a/demo/next.config.js b/demo/next.config.js
index 5b7a483..95f4633 100644
--- a/demo/next.config.js
+++ b/demo/next.config.js
@@ -4,6 +4,9 @@ module.exports = {
   webpack: (config, options) => {
     if (options.isServer) {
       config.externals = ['react', 'theme-ui', ...config.externals]
+      config.resolve.mainFields = ['module', 'main'];
+    } else {
+      config.resolve.mainFields = ['browser', 'module', 'main'];
     }
     config.resolve.alias['react'] = path.resolve(
       __dirname,

I believe if carbonplan/maps switched to shipping ESM only as well it should not be an issue. For code that is intended to run in the browser-like environments, I don't see the need to CJS since it is node-specific and bundlers all target ESM. Happy to help with that transition if it is of interest.

@katamartin
Copy link
Collaborator Author

@manzt thanks for looking into this and digging into our build steps 😅 !

I've had to put this down to work on other things, but I did quickly try updating the Next config in a different consumer repo and couldn't get it working. I started to play with transitioning to ESM based on your suggestion, but ran out of time to fully test that out. Hoping to get back to this soon with these breadcrumbs -- thanks again for all of the help!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants