search

hms-dbmi / viv

Library for multiscale visualization of high-resolution multiplexed bioimaging data on the web. Directly renders Zarr and OME-TIFF.
http://avivator.gehlenborglab.org
MIT License
282 stars 45 forks source link

package.json points to dist file that trips up Next.js #303

Closed andreasg123 closed 3 years ago

andreasg123 commented 3 years ago

In package.json, both main and module point to the same file containing import statements. The import statements probably come from this line: https://github.com/hms-dbmi/viv/blob/master/rollup.config.js#L19

Unfortunately, that trips up Next.js. It hides its Webpack configuration fairly well so that I'm not completely sure what it's doing exactly. I worked around it with next-transpile-modules but that plugin describes itself as "a big hack of the Next.js Webpack configuration".

My understanding is that module is supposed to point to esm and main is supposed to point to es5. At least, that's what deck.gl/core does (with esnext as a third format): https://github.com/visgl/deck.gl/blob/master/modules/core/package.json#L19

I'm not familiar with rollup but I could attempt to produce a PR if you don't get around to it first.

ilan-gold commented 3 years ago

@andreasg123 I was actually recently trying this package in a Next.js setup with no issue, other than needing to add

webpack: (config, { isServer }) => {
    // Fixes npm packages that depend on `fs` module
    if (!isServer) {
      config.node = {
        fs: 'empty'
      }
    }
    return config
  }

to the config. @manzt I remember asking something similar, but I tried this recently with vizarr and didn't have this issue. Do you know what is happening here?

manzt commented 3 years ago

Are you able to describe the issue in more detail? An error message would be useful. I'm not sure what is meant by "trips up Next.js," but you shouldn't need to transpile Viv (or any of your node_modules) to be able to use them in your build.

My understanding is that module is supposed to point to esm and main is supposed to point to es5.

This is specific to deck.gl. The module field in package.json isn't an official field, and instead something that's been adopted by JS package creators to signify a pure ESM export of their package. The main field is official and meant to signify the default entry point for your package. In our case, the ESM bundle is our default export because viv is intended to be used with bundlers.

Webpack supports ESM natively. It is the most simple format for bundlers to resolve, which is why the default fields webpack tries to resolve are browser, module, main (https://webpack.js.org/configuration/resolve/#resolveimportsfields).

TL;DR - My guess is that the error you're experiencing has to do with next.js trying to resolve an import from a Node library like @ilan-gold pointed out ("fs"), and not identical fields in the package.json. My bet is that next-transpile-modules does something similar to @ilan-gold suggestion internally when transpiling Viv's source to a different format.

andreasg123 commented 3 years ago

I also ran into the "fs" error with "geotiff" and fixed it the same way.

I will attempt to determine which of the following may produce a different behavior for me when using Next.js:

Here is the error that I'm getting:

yarn run v1.22.10
$ next build && rm -rf out && next export
info  - Using external babel configuration from .../client/babel.config.js
info  - Creating an optimized production build  
info  - Compiled successfully

> Build error occurred
.../client/node_modules/@hms-dbmi/viv/dist/index.js:1
import { Layer, project32, picking, COORDINATE_SYSTEM, CompositeLayer, OrthographicView } from '@deck.gl/core';
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at wrapSafe (internal/modules/cjs/loader.js:979:16)
    at Module._compile (internal/modules/cjs/loader.js:1027:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.Module._load (internal/modules/cjs/loader.js:769:14)
    at Module.require (internal/modules/cjs/loader.js:952:19)
    at require (internal/modules/cjs/helpers.js:88:18)
    at Object.yR3o (.../client/.next/server/pages/index.js:244:18)
    at __webpack_require__ (.../client/.next/server/pages/index.js:23:31)
    at Module.23aj (.../client/.next/server/pages/index.js:126:12) {
  type: 'SyntaxError'
}
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
manzt commented 3 years ago

Thanks for updating with more detailed information! What version of node/npm are you using?

Could you do me a favor and delete the "main" field from node_modules/@hms-dbmi/viv/package.json and try building again? I'm curious if that because it detects a main field, the webpack config will try to load that using a commonjs loader. We could create a cjs export of viv easily with our rollup config, but since Viv is intended to run in the browser I'd like to see if we can find a solution without going that route (since that cjs variant will just be converted back to some type of browser-compatible format by webpack).

andreasg123 commented 3 years ago

Without the main field, it runs into an error much earlier:

$ next build
Failed to compile.

./components/ImageViewer/index.tsx:1:61
Type error: Cannot find module '@hms-dbmi/viv' or its corresponding type declarations.

> 1 | import { PictureInPictureViewer, createOMETiffLoader } from "@hms-dbmi/viv";
    |                                                             ^
  2 | import { useEffect, useState } from "react";
  3 | 
  4 | export default function ImageViewer() {
error Command failed with exit code 1.

I'm using Node 14.15.1 and Yarn 1.22.10. I'm using next 10.0.2 and React 16.13.1 in case that makes a difference.

If you can suggest changes in next.config.js that would get Webpack to use esm, I would be happy to try those.

manzt commented 3 years ago

Darn, thanks for trying it out! good to know. Let me look into the next configuration...

If you don't mind, could you add back: "main": "dist/index.js" along with a new field "type": "module". That's the only think I can think of right now (without digging into next more).

andreasg123 commented 3 years ago

Unfortunately, that didn't work, either:

$ next build
info  - Using external babel configuration from .../client/babel.config.js
info  - Creating an optimized production build  
info  - Compiled successfully

> Build error occurred
Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: .../client/node_modules/@hms-dbmi/viv/dist/index.js
require() of ES modules is not supported.
require() of .../client/node_modules/@hms-dbmi/viv/dist/index.js from .../client/.next/server/pages/index.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename .../client/node_modules/@hms-dbmi/viv/dist/index.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from .../client/node_modules/@hms-dbmi/viv/package.json.

    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1080:13)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.Module._load (internal/modules/cjs/loader.js:769:14)
    at Module.require (internal/modules/cjs/loader.js:952:19)
    at require (internal/modules/cjs/helpers.js:88:18)
    at Object.yR3o (.../client/.next/server/pages/index.js:244:18)
    at __webpack_require__ (.../client/.next/server/pages/index.js:23:31)
    at Module.23aj (.../client/.next/server/pages/index.js:126:12)
    at __webpack_require__ (.../client/.next/server/pages/index.js:23:31)
    at Object.2 (.../client/.next/server/pages/index.js:99:18) {
  type: 'NodeError',
  code: 'ERR_REQUIRE_ESM'
}
error Command failed with exit code 1.

In the meantime, I tried to add "type": "module" in my own package.json, kind of following suggestions from here: https://github.com/vercel/next.js/issues/9607

However, I had to change next.config.js and babel.config.js to .cjs. The Next config file wasn't picked up so that I ran into the "fs" problem again.

I wonder if my use of TypeScript is the main issue here (or that I need Babel for Jest).

andreasg123 commented 3 years ago

As other modules (e.g., GitHub) switch to ESM-only output, I think that it would be fine if you left things as they are. Maybe Next.js will handle this more gracefully in the future. Maybe you could note it in your README.

ilan-gold commented 3 years ago

@andreasg123 We recently changed out build system to Vite - did that help at all with this? Can we close this?

andreasg123 commented 3 years ago

Let me check tomorrow and get back to you.

andreasg123 commented 3 years ago

Unfortunately, I cannot include Viv 0.10.5 at all, without or with transpiling. I'll investigate this further and get back to you.

Error with Viv 0.10.3 without transpiling:

> Build error occurred
path/node_modules/@hms-dbmi/viv/dist/index.js:1
import { COORDINATE_SYSTEM, Layer, project32, picking, CompositeLayer, OrthographicView, Controller, OrbitView } from '@deck.gl/core';
^^^^^^
SyntaxError: Cannot use import statement outside a module
    at Object.compileFunction (node:vm:355:18)
    at wrapSafe (node:internal/modules/cjs/loader:1021:15)
    at Module._compile (node:internal/modules/cjs/loader:1055:27)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1120:10)
    at Module.load (node:internal/modules/cjs/loader:971:32)
    at Function.Module._load (node:internal/modules/cjs/loader:812:14)
    at Module.require (node:internal/modules/cjs/loader:995:19)
    at require (node:internal/modules/cjs/helpers:92:18)
    at Object.yR3o (path/.next/server/pages/index.js:8752:18)
    at __webpack_require__ (path/.next/server/pages/index.js:23:31) {
  type: 'SyntaxError'
}

Error with Viv 0.10.5 without transpiling:

./node_modules/@hms-dbmi/viv/dist/viv.es.js
Module parse failed: Unexpected token (284:29)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
|  */
| function getPhysicalSizeScalingMatrix(loader) {
>   const { x, y, z } = loader?.meta?.physicalSizes ?? {};
|   if (x?.size && y?.size && z?.size) {
|     const min = Math.min(z.size, x.size, y.size);
> Build error occurred
Error: > Build failed because of webpack errors
    at path/node_modules/next/dist/build/index.js:17:924
    at async Span.traceAsyncFn (path/node_modules/next/dist/telemetry/trace/trace.js:6:584)

Error with Viv 0.10.5 with transpiling:

> Build error occurred
ReferenceError: Blob is not defined
    at Module.QeBL (path/.next/server/pages/index.js:6271:6280)
    at __webpack_require__ (path/.next/server/pages/index.js:23:31)
    at Object.4 (path/.next/server/pages/index.js:167:18)
    at __webpack_require__ (path/.next/server/pages/index.js:23:31)
    at path/.next/server/pages/index.js:91:18
    at Object.<anonymous> (path/.next/server/pages/index.js:94:10)
    at Module._compile (node:internal/modules/cjs/loader:1091:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1120:10)
    at Module.load (node:internal/modules/cjs/loader:971:32)
    at Function.Module._load (node:internal/modules/cjs/loader:812:14) {
  type: 'ReferenceError'
andreasg123 commented 3 years ago

I created a PR for Vite: https://github.com/vitejs/vite/pull/4674

I manually modified viv.es.js just as my PR would do. That made it work with Next.js.

ilan-gold commented 3 years ago

Thanks! Hopefully that PR lands soon and we can upgrade.

manzt commented 3 years ago

My confusion here is that viv is a purely client-side library; the module isn't intended to be run on the server. The part of the code with Blob is used to instantiate another Web-API not supported in Node natively, WebWorker, so it surprising to me that somehow Worker is defined in the next.js server context as a global.

Have you looked into disabling SSR for Viv, or injecting Blob as a global in next.js? https://nodejs.org/dist/latest-v16.x/docs/api/buffer.html#buffer_class_blob

Eg with getStaticProps: https://github.com/vercel/next.js/discussions/17404#discussioncomment-86126