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

import of particular client side javascript dependency fails #401

Closed
max-l opened this issue Mar 24, 2023 · 19 comments
Closed

import of particular client side javascript dependency fails #401

max-l opened this issue Mar 24, 2023 · 19 comments
Labels
bug Something isn't working

Comments

@max-l
Copy link

max-l commented Mar 24, 2023

Version

1.16.0

Platform

deno 1.30.3 (release, x86_64-unknown-linux-gnu) v8 10.9.194.5 typescript 4.9.4

What steps will reproduce the bug?

When importing a package from "deno:https://esm.sh/react-table@7.8.0"

with the following code:

import { useExpanded, useTable } from 'npm:react-table';

I get the following error:

✘ [ERROR] No matching export in "deno:https://esm.sh/react-table@7.8.0" for import "useExpanded"

deno:file:///home/maxou/dev/site-altorf/altorf/python-lib/openprot_pipeline/site/app/SearchScreen.jsx:3:9:
  3 │ import { useExpanded, useTable } from 'npm:react-table@7.8.0';
    ╵          ~~~~~~~~~~~

The exports are defined, as shown here (in the same version, i.e. 7.8.0): https://react-table-v7.tanstack.com/docs/examples/sub-components-lazy

This is probably an issue with the library not being packaged to Deno's (or esbuild's) taste, but I was wondering if there are
other means to declare dependencies, where more "dependency incantation tweaking" is possible, I'm guessing
import_map.json would probably be the place to do that, but I'm wondering if this map file only concerns imports for Lume's static/build time rendering, or if I can do some dependency tweaking for esbuild in there.

How often does it reproduce? Is there a required condition?

reproduces with:

git clone https://github.com/max-l/react-todo/blob/main/app/app.tsx
cd react-todo
deno task lume --serve 

What is the expected behavior?

proper import of module

What do you see instead?

error message in stdout

Additional information

No response

@max-l max-l added the bug Something isn't working label Mar 24, 2023
@oscarotero
Copy link
Member

oscarotero commented Mar 24, 2023

Hi.
I have been fighting with the esbuild plugin to provide better support for NPM packages and just upload some changes. You can test it using the last development version of Lume (deno task lume upgrade --dev).

The main change was to switch from esm.sh to unpkg.com as CDN to serve the NPM dependencies. This ensure the code processed by esbuild is the same code provided by NPM instead of a ES module version transpiled by esm.sh.

With this change I could finally swich to development mode in React. For example, the following code bundles React JSX code to production:

site.use(esbuild({
    extensions: [".jsx"],
    options: {
      jsxDev: false,
      define: {
        "process.env.NODE_ENV": "'production'",
      }
    }
}))

And this code is for development:

site.use(esbuild({
    extensions: [".jsx"],
    options: {
      jsxDev: true,
    }
}))

So this change should fix #400.

The only problem is unpkg.com doesn't resolve the dependencies versions. For example, the code:

const react = require("react");

It doesn't contain the react version, so by default it goes to unpkg.com/react, returning always the latest version. A solution could be provide an option to define the dependencies versions manually, for example:

site.use(esbuild({
  dependencies: {
    react: "18.2.0"
  }
}));

But not sure if this is a good idea.

I'm guessing import_map.json would probably be the place to do that, but I'm wondering if this map file only concerns imports for Lume's static/build time rendering, or if I can do some dependency tweaking for esbuild in there.

esbuild uses the same import map as Deno to resolve the dependencies.

@oscarotero
Copy link
Member

✘ [ERROR] No matching export in "deno:https://esm.sh/react-table@7.8.0" for import "useExpanded"

This is the typical error when converting a CJS module to ESM. Probably this doesn work:

import { useExpanded } from 'npm:react-table';

but this does:

import reactTable from 'npm:react-table';
const { useExpanded } = reactTable;

@max-l
Copy link
Author

max-l commented Mar 24, 2023

I tried this and got :

main.js:20 Uncaught ReferenceError: Deno is not defined
    at main.js:20:3057

The line is half way in the generated bundle:

Deno.build.os === "darwin" ? xn = {
    UV_UDP_REUSEADDR: 4,
    dlopen: {
        RTLD_LAZY: 1,
        RTLD_NOW: 2,

I suppose the code should be stripped away, but it isn't, or maby a shim or polyfill needs to be added.

@oscarotero
Copy link
Member

Are you using the Deno namespace in your client-side javascript code?

@max-l
Copy link
Author

max-l commented Mar 24, 2023

Nothing on the client refers to deno,

the only thing in the whole project related to deno (indirectly) is this line :

import { Helper } from "lume/core.ts";

https://github.com/max-l/react-todo/blob/main/index.tmpl.ts

@max-l
Copy link
Author

max-l commented Mar 24, 2023

I managed to get the import working with this :

import {useTable} from 'npm:react-table?cjs-exports=useTable'

as recomended on https://esm.sh/#docs

Specify CJS Exports
If you get an error like ...not provide an export named..., that means esm.sh can not resolve CJS exports of the module correctly. You can add ?cjs-exports=foo,bar query to specify the export names:

import { NinetyRing, NinetyRingWithBg } from "https://esm.sh/react-svg-spinners@0.3.1?cjs-exports=NinetyRing,NinetyRingWithBg"

But I still get the error "Uncaught ReferenceError: Deno is not defined"

@oscarotero
Copy link
Member

oscarotero commented Mar 24, 2023

Ok, it seems it comes from namor dependency. It uses Node APIs (like crypto) so it's not browser friendly.

@oscarotero
Copy link
Member

As commented in #400 (comment) the new esmOptions key configures the options to send to esm.sh, so you don't have to include it in your code.

@max-l
Copy link
Author

max-l commented Mar 24, 2023

Great, I think this issue can be closed, as it's a esm.sh problem that has the "?cjs-exports" workaround.

Thanks !

@max-l
Copy link
Author

max-l commented Mar 27, 2023

I'm now getting a strane error related to "?cjs-exports",

the following import :

import { useExpanded, useTable } from 'npm:react-table?cjs-exports=useTable,useExpanded';

works file if I add it while lume is serving (deno task lume -s )
If I stop it, and then restart it, I then get the error below.

I tried reproducing the problem in this repo https://github.com/max-l/react-todo/blob/main/app/TestTable.jsx

but unfortunately I'm not able to get the bug to occur in this repo.

deno task lume -s 
Task lume echo "import 'lume/cli.ts'" | deno run --unstable -A - "-s"
Loading config file file:///home/maxou/dev/site-altorf/altorf/python-lib/openprot_pipeline/site/_config.ts


Error: Couldn't load this file
- fullPath: /home/maxou/dev/site-altorf/altorf/python-lib/openprot_pipeline/site/index.tmpl.ts
    at Reader.read (https://deno.land/x/lume@v1.16.0/core/reader.ts:156:13)
    at async PageLoader.load (https://deno.land/x/lume@v1.16.0/core/page_loader.ts:49:18)
    at async Source.#loadEntry (https://deno.land/x/lume@v1.16.0/core/source.ts:431:22)
    at async concurrent (https://deno.land/x/lume@v1.16.0/core/utils.ts:181:3)
    at async Source.load (https://deno.land/x/lume@v1.16.0/core/source.ts:227:12)
    at async Site.build (https://deno.land/x/lume@v1.16.0/core/site.ts:511:5)
    at async build (https://deno.land/x/lume@v1.16.0/cli/build.ts:36:3)
    at async Command.execute (https://deno.land/x/cliffy@v0.25.7/command/command.ts:1794:7)
    at async Command.parseCommand (https://deno.land/x/cliffy@v0.25.7/command/command.ts:1639:14)

Caused by TypeError: Relative import path "react" not prefixed with / or ./ or ../ and not in import map from "file:///home/maxou/dev/site-altorf/altorf/python-lib/openprot_pipeline/site/app/ProteinTable.jsx"
    at async module (https://deno.land/x/lume@v1.16.0/core/loaders/module.ts:8:15)
    at async Reader.read (https://deno.land/x/lume@v1.16.0/core/reader.ts:154:14)
    at async PageLoader.load (https://deno.land/x/lume@v1.16.0/core/page_loader.ts:49:18)
    at async Source.#loadEntry (https://deno.land/x/lume@v1.16.0/core/source.ts:431:22)
    at async concurrent (https://deno.land/x/lume@v1.16.0/core/utils.ts:181:3)
    at async Source.load (https://deno.land/x/lume@v1.16.0/core/source.ts:227:12)
    at async Site.build (https://deno.land/x/lume@v1.16.0/core/site.ts:511:5)
    at async build (https://deno.land/x/lume@v1.16.0/cli/build.ts:36:3)
    at async Command.execute (https://deno.land/x/cliffy@v0.25.7/command/command.ts:1794:7)

@oscarotero
Copy link
Member

I can't reproduce the error. That's weird:
imaxe

@oscarotero
Copy link
Member

Looking at the error message:

Caused by TypeError: Relative import path "react" not prefixed with / or ./ or ../ and not in import map from "file:///home/maxou/dev/site-altorf/altorf/python-lib/openprot_pipeline/site/app/ProteinTable.jsx"

The error comes from the file:///home/maxou/dev/site-altorf/altorf/python-lib/openprot_pipeline/site/app/ProteinTable.jsx file, but this file does not exist in the repo.

It looks like you're importing reactwithout the npm: prefix somewhere.

@max-l
Copy link
Author

max-l commented Mar 28, 2023

ProteinTable.jsx is a file in my repo, I was trying to reproduce the problem in https://github.com/max-l/react-todo
but wasn't able to.

I will put my Lume project in a public repo so you can see the whole thing. I can assure you there are no "react" without the "npm:" prefix, but perhaps I'm doing something else just as dumb ;-)

max-l added a commit to max-l/lume-cjs-exports-test-case that referenced this issue Mar 29, 2023
@max-l
Copy link
Author

max-l commented Mar 29, 2023

@oscarotero I managed to extract the code to reproduce the problem, in this repo: https://github.com/max-l/lume-cjs-exports-test-case

@oscarotero
Copy link
Member

@max-l Ok thanks.
I was testing the repo and indeed, it fails. But the problem is not esbuild (that works fine) but the server side rendering (by Deno).

I discovered the problem is this line https://github.com/max-l/lume-cjs-exports-test-case/blob/master/app/ProteinTable.jsx#L25 (if I remove it, it works fine).

Moving the line to after the key attribute also fix the error:

<tr
  key={`${rowProps.key}-expanded-${i}`}
  {...rowProps}
>

Don't ask me why :D
Maybe it's a bug of Deno or maybe in JSX the variadic attributes must be placed at the end (like in JS functions).

@max-l
Copy link
Author

max-l commented Mar 31, 2023

Interesting !

I'm a bit puzzled that Deno is involved here, because that code never runs on the server (or at least it shouldn't), if esm does the compilation, the only thing Deno should do is to send the code (as inert data) to the client.

I noticed something interesting: if I remove the offending code, start Lume, and then add the code again, the code reloads and it works correctly.

It seems as if the offending code prevents something on the server to initialize correctly, because once you remove it, and the initialisation gets done, the offending code has lost it's corrupting effect !

@oscarotero
Copy link
Member

The to-do app repository (that you used as a template) pre-renders the code at buildtime in order to be hydrated later on client side.

At buildtime, the App is imported in the index.tmpl.ts page and rendered here: https://github.com/max-l/lume-cjs-exports-test-case/blob/master/index.tmpl.ts#L6

On the client side, the app is imported and hydrated here: https://github.com/max-l/lume-cjs-exports-test-case/blob/master/main.jsx#L6-L11

So if you don't need buildtime pre-rendering, you can remove the code in the index.tmpl.ts file.

@max-l
Copy link
Author

max-l commented Mar 31, 2023

Interesting, there was much more magic going on than I thought !

I suppose I should use useEffect to switch on/off the prerendering strategicaly ?

@oscarotero
Copy link
Member

oscarotero commented Mar 31, 2023

The prerendering is handled in the Lume template, before any React code is executed.

There's no much magic: just render the root component App with ReactDOMServer.renderToString() function and place the HTML result in the page (here: https://github.com/max-l/lume-cjs-exports-test-case/blob/master/index.tmpl.ts#L20).

The hydration step on the browser side just complete the HTML adding the event listeners (onclick, etc). It only makes sense if the component's data is available at buildtime, so the output HTML is similar to the code generated on the client side. In the to-do example, it didn't make sense because the data is stored in the localStorage on the browser side, so at build time only generates an empty to-do list. But it's just an example to demostrate that this is possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants