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

[🐞] Duplicate implementations of JSXNode Errors in RC, v0.103 #3883

Open
n8sabes opened this issue Apr 21, 2023 · 31 comments · May be fixed by #7109
Open

[🐞] Duplicate implementations of JSXNode Errors in RC, v0.103 #3883

n8sabes opened this issue Apr 21, 2023 · 31 comments · May be fixed by #7109
Labels
hacktoberfest STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working

Comments

@n8sabes
Copy link
Contributor

n8sabes commented Apr 21, 2023

Which component is affected?

Qwik Runtime

Describe the bug

I'm getting QWIK WARN Duplicate implementations of "JSXNode" found warnings when using Qwik Styled Vanilla Extract.

This happens with RC1 Starter, following Integration Docs. Please see Loom Example, if needed.

Also, with v1.0 around the corner, there are open PRs and Issues with styled-vanilla-extract that should get a little love since it's listed in the official documentation integrations section.

Cc: @manucorporat, @wmertens

Reproduction

https://www.loom.com/share/9eab1daa92f0431d848f35ea67899258

Steps to reproduce

pnpm create qwik@latest
pnpm qwik add styled-vanilla-extract
pnpm dev

System Info

System:
    OS: macOS 13.3.1
    CPU: (8) arm64 Apple M1
    Memory: 65.23 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.0.0 - ~/.nvm/versions/node/v20.0.0/bin/node
    npm: 8.19.2 - /opt/homebrew/bin/npm
  Browsers:
    Chrome: 112.0.5615.137
    Firefox Developer Edition: 111.0
    Safari: 16.4
  npmPackages:
    @builder.io/qwik: 0.103.0 => 0.103.0
    @builder.io/qwik-city: ~0.103.0 => 0.103.0
    undici: 5.21.2 => 5.21.2
    vite: 4.2.2 => 4.2.2

Additional Information

No response

@n8sabes n8sabes added TYPE: bug Something isn't working STATUS-1: needs triage New issue which needs to be triaged labels Apr 21, 2023
@n8sabes n8sabes changed the title [🐞] Duplicate implementations of JSXNode Errors in RC 103 [🐞] Duplicate implementations of JSXNode Errors in RC, v0.103 Apr 21, 2023
@brandiqa
Copy link

I've come across this issue in v1.1.1 and v1.0.0. when using the library @qwikest/icons

@NiklasPor
Copy link
Contributor

FYI @n8sabes , I've managed to "fix" this by merging all my original entry points into a single one. But after this the qwik compiler is too heavy for my machine and the build fails because it runs out of heap (~about 24k qwik components)

@wmertens
Copy link
Member

wmertens commented May 12, 2023

@NiklasPor do you really need component$s? Or could lite components suffice?

Then maybe you can skip the build step and just export functions that call jsx?

@NiklasPor
Copy link
Contributor

NiklasPor commented May 12, 2023

Hm, about one third of the components use a context hook and if I remember correctly they don't work in lite / inline components. I'll consider throwing away the feature which uses the context, so that I atleast can publish a stable version 👍

Still I think it would be great if we could maybe reach a point in the future, where it's possible to add multiple entrypoints, just like with a regular vite config / js package. A lot of large packages in the JS ecosystem use the functionality. Probably a possibility for later point on some roadmap 👀

@wmertens
Copy link
Member

You could also only build one lite and one regular component and use those as templates for the rest

@NiklasPor
Copy link
Contributor

NiklasPor commented May 12, 2023

Lite components seem to work fine. Just in dev mode (vite --mode ssr) no tree-shaking is happening, so on the first user interaction with one of the lite components all of the JS loaded. Preview mode tree-shaking works and performance is superb as always 😁

@NiklasPor
Copy link
Contributor

Alright, so the icon package now works with multiple entrypoints + only lite components – but the warning is now back that I'm using multiple entrypoints. Thanks for the suggestions @wmertens

kisaragi-hiu added a commit to changelog-db/changelog-db that referenced this issue May 15, 2023
@hbendev
Copy link
Contributor

hbendev commented May 30, 2023

The warning is still there when using @qwikest/icons 0.0.8 together with qwik 1.1.0 or qwik 1.1.4 and using import { HiXY } from "@qwikest/icons/heroicons".

@chempogonzalez
Copy link

Hi! I have seen the workaround commited from @kisaragi-hiu referencing this issue but it removes completely all errors thrown to the shell.

As a temporal workaround to avoid the noise caused using @qwikest/icons with all these warnings in console, I'm using the following script on top of the entry.ssr.tsx. By this way we don't loose other console outputs. I hope it helps you @hbendev :)

// entry.ssr.tsx
import { isDev } from '@builder.io/qwik/build'

if (isDev) {
  const consoleWarn = console.warn
  const SUPPRESSED_WARNINGS = ['Duplicate implementations of "JSXNode" found']

  console.warn = function filterWarnings (msg, ...args) {
    if (!SUPPRESSED_WARNINGS.some(entry => msg.includes(entry) || args.some(arg => arg.includes(entry))))
      consoleWarn(msg, ...args)
  }
}

@noc2spam
Copy link

noc2spam commented Jul 9, 2023

Hi! I have seen the workaround commited from @kisaragi-hiu referencing this issue but it removes completely all errors thrown to the shell.

As a temporal workaround to avoid the noise caused using @qwikest/icons with all these warnings in console, I'm using the following script on top of the entry.ssr.tsx. By this way we don't loose other console outputs. I hope it helps you @hbendev :)

// entry.ssr.tsx
import { isDev } from '@builder.io/qwik/build'

if (isDev) {
  const consoleWarn = console.warn
  const SUPPRESSED_WARNINGS = ['Duplicate implementations of "JSXNode" found']

  console.warn = function filterWarnings (msg, ...args) {
    if (!SUPPRESSED_WARNINGS.some(entry => msg.includes(entry) || args.some(arg => arg.includes(entry))))
      consoleWarn(msg, ...args)
  }
}

Works fine, thank you. :)

However, I guess solving the problem actually would be a better solution at the end of the day. :)

@wmertens
Copy link
Member

wmertens commented Jul 9, 2023

I'd like to add that it seems like the error is harmless in the case of the icons package, but it can lead to actual things not working, see #4436

@NiklasPor
Copy link
Contributor

NiklasPor commented Jul 9, 2023

I'd like to add that it seems like the error is harmless in the case of the icons package, but it can lead to actual things not working, see #4436

I very strongly second this, the only reasons the icons package is working, is that I'm not using any component$ inside there, but instead use regular function components / lite components. Otherwise the package would break during server side rendering afaik.

If I'd need to guess, the reason seems to be connected to the multiple entrypoints. Probably qwik is only perprocessing specfic entrypoints?

@Mo0nbase
Copy link

Has there been any progress on this issue? I currently cannot use the icons package because It is breaking parts of my app.

@mhevery
Copy link
Contributor

mhevery commented Sep 27, 2023

So this error means that Vite has somehow packaged up the JSX code twice! And this is an early error so that you don't go nuts later with strange behavior.... So if you see this error, the question you should be asking is why are there two copies of JSX implementation in the system. This is not an issue with Qwik, but rather how you have the vite configured....

If someone can give me a simple reproduction that you think should be fixed, I will look into it.

@wmertens
Copy link
Member

@mhevery I posted a repro in #4436, https://stackblitz.com/edit/qwik-starter-wadzsa?file=src/routes/index.tsx

it happens in dev mode too, and it breaks in production. Qwik is definitely packaged only once. The error indicates that the JSXNodeImpl class is instantiated twice, but I can't figure out how.

@NiklasPor
Copy link
Contributor

NiklasPor commented Sep 27, 2023

@mhevery I posted a repro in #4436, https://stackblitz.com/edit/qwik-starter-wadzsa?file=src/routes/index.tsx

it happens in dev mode too, and it breaks in production. Qwik is definitely packaged only once. The error indicates that the JSXNodeImpl class is instantiated twice, but I can't figure out how.

I'm pretty sure this happens, because you also use multiple named exports in the package.json. With @qwikest/icons I did not get any issues when I merged all code behind a single default export (sadly this wasn't an option for the package).

Maybe there's something hardcoded in Qwik which looks exactly for an entrypoint . like mentioned in the qwik lib docs?

    ".": {
      "import": "./lib/index.qwik.mjs",
      "require": "./lib/index.qwik.cjs",
      "types": "./lib-types/index.d.ts"
    }

The docs also impose some restrictions on how the file must be named:

The file must be called with the .qwik.mjs extension, otherwise the Qwik Optimizer will not recognize it.

In styled-vanilla-extract it looks like this:

    "./qwik": {
      "import": "./lib/index.js",
      "require": "./lib/index.cjs",
      "types": "./lib-types/index.d.ts"
    },
    "./vite": {
      "import": "./lib/vite.js",
      "require": "./lib/vite.cjs",
      "types": "./lib-types/vite.d.ts"
    },
    "./qwik-styled": {
      "import": "./lib/qwik-styled.js",
      "require": "./lib/qwik-styled.cjs"
    },
    "./package.json": "./package.json"

I would love it if we get to support multi-entrypoint packages.

@wmertens
Copy link
Member

@NiklasPor well you just solved why qwik-ui doesn't work 😅 cc @shairez

I'll see if I can fix styled-vanilla-extract.

I'm assuming the duplicated JSXNodeImpl is because of the optimizer bundling it directly and then vite bundling it separately where it wasn't done by the optimizer.

If so, the optimizer could instead look at the imports and see if qwik is imported instead of only relying on the filename

@NiklasPor
Copy link
Contributor

Always happy to help, I'd love it when we get the packages to work without problems – kinda important for the ecosystem 😁

@wmertens
Copy link
Member

I upgraded the repro to latest everything and built styled-vanilla-extract with .qwik.mjs extensions, but it didn't help.

https://stackblitz.com/edit/qwik-starter-8kdqva?file=package-lock.json,package.json

@wmertens
Copy link
Member

I figured it out. In the repro above I added console.log(new Error('loading qwik [prod] [mjs|cjs]')) in the node_modules qwik core files, and indeed qwik gets loaded multiple times.

What happens is: Vanilla-extract runs the css.ts files at build time and generates a file that imports styled-vanilla-extract/qwik-styled which in turn imports qwik.
https://github.com/wmertens/styled-vanilla-extract/blob/4f03da0fc8d53d2f6bd19a52e523c4c7f30571ee/src/ve-style.ts#L44

This generated import doesn't get bundled by vite. You can see it at the top of server/entry.preview.mjs:

import { styled } from "styled-vanilla-extract/qwik-styled";

I added the vite inspect plugin to the repro so you can see all the transforms. Everything seems correct, but the dev build does import core.prod.mjs even though I can't find what imports it.

For dev mode, I assume the same thing is happening.


So the problem is either:

  • vite is bundling qwik into the server entry, or
  • vite is not bundling styled-vanilla-extract as well

I think it would be safer if the server build did not bundle qwik

@wmertens
Copy link
Member

BTW @NiklasPor not that it seems to make a difference, but your package.json does not include the "qwik" exports which the docs say are required. (your file naming is ok)

@wmertens
Copy link
Member

Ok I solved it.

This is what happens:

  • the qwikVite plugin forces bundling of Qwik
  • vanilla-extract generates an import from an external which doesn't get bundled
  • but it contains an import of qwik
  • so then it will have a duplicate entry of qwik
  • duplicate spidermen pointing.jpeg
  • ...and this is already thought of! In that same line, it includes vendorIds
  • ...they are found by checking all the dependencies for package.json files that have qwik as a key!

So by having a key "qwik" at the root of your package.json, it will be bundled into the ssr build, which fixes the duplicate jsxnode.

This is somewhat documented, so it's a matter of making it more clear.

Ideally, the node_modules qwik is somehow poisoned with an error so that it can be caught and corrected easily.

I do think it's a bit confusing to have the "qwik" property be required and it not seemingly doing anything. It's also not documented what it needs to point to - what if you have multiple entry points for qwik code?

So @NiklasPor add the "qwik" attribute to your package.

@mhevery @manucorporat what should the "qwik" attribute point at, exactly? And how to make it easier to discover that ".qwik.js" wasn't generated or that the "qwik" attribute is missing?

@NiklasPor
Copy link
Contributor

Awesome @wmertens, I'll check If I can finally fix the package with this! 😅

@NiklasPor
Copy link
Contributor

@wmertens Seems like my local testing everything went fine with this, can't believe that this small thing made the change 🤯 I'll check if it works with the community, thanks a lot ❤️ 0.0.9 of @qwikest/icons includes the change, I'm just pointing to an empty entrypoint 😆

@mhevery
Copy link
Contributor

mhevery commented Oct 5, 2023

@mhevery @manucorporat what should the "qwik" attribute point at, exactly? And how to make it easier to discover that ".qwik.js" wasn't generated or that the "qwik" attribute is missing?

Maybe I am misunderstanding the question, but I think the package.json should just be

{
  "qwik": true
}

@mhevery
Copy link
Contributor

mhevery commented Oct 5, 2023

+1 to better document this. Also maybe add a section to the documentation explaining a common failure mode and then update the error with a pointer to the documentation saying maybe this is what is going on? Any chance @wmertens or someone could create a PR for that? Please 🍒 on top?

@wmertens
Copy link
Member

wmertens commented Oct 5, 2023

Maybe I am misunderstanding the question, but I think the package.json should just be

{
  "qwik": true
}

I did try that but it complains somewhere. It expects a string. Presumably it's used elsewhere, the vendorIds just use it as a truthy value.

@mhevery
Copy link
Contributor

mhevery commented Oct 5, 2023

What is the error?

@mhevery
Copy link
Contributor

mhevery commented Oct 5, 2023

OK,

I think the code is here: https://github.com/BuilderIO/qwik/blob/main/packages/qwik/src/optimizer/src/plugins/vite.ts#L704
And the correct value should be a pointer to the MJS file: "qwik": "./lib/index.qwik.mjs",

@wmertens
Copy link
Member

wmertens commented Oct 5, 2023

What is the error?

TypeError [ERR_INVALID_ARG_TYPE]: The "paths[1]" argument must be of type string. Received type boolean (true)
    at __node_internal_ (https://qwikstarter8kdqva-clg3.w-credentialless.staticblitz.com/blitz.5e765aa0.js:36:5410)
    at new <anonymous> (https://qwikstarter8kdqva-clg3.w-credentialless.staticblitz.com/blitz.5e765aa0.js:36:4172)
    at validateString (https://qwikstarter8kdqva-clg3.w-credentialless.staticblitz.com/blitz.5e765aa0.js:38:1232)
    at Object.resolve (https://qwikstarter8kdqva-clg3.w-credentialless.staticblitz.com/blitz.5e765aa0.js:44:7412)
    at eval (file:///home/projects/qwik-starter-8kdqva/node_modules/@builder.io/qwik/optimizer.cjs:3445:34)
    at Array.map (<anonymous>)
    at findQwikRoots (file:///home/projects/qwik-starter-8kdqva/node_modules/@builder.io/qwik/optimizer.cjs:3438:37)
    at async config (file:///home/projects/qwik-starter-8kdqva/node_modules/@builder.io/qwik/optimizer.cjs:3073:49)
    at async runConfigHook (file:///home/projects/qwik-starter-8kdqva/node_modules/vite/dist/node/chunks/dep-df561101.js:66324:25)
    at async resolveConfig (file:///home/projects/qwik-starter-8kdqva/node_modules/vite/dist/node/chunks/dep-df561101.js:65767:14)
    at async _createServer (file:///home/projects/qwik-starter-8kdqva/node_modules/vite/dist/node/chunks/dep-df561101.js:65011:20)
    at async CAC.eval (file:///home/projects/qwik-starter-8kdqva/node_modules/vite/dist/node/cli.js:758:24) {
  code: 'ERR_INVALID_ARG_TYPE'
}

...which is indeed inside the code you point at.

And the vendorRoots are used by the optimizer as input file: https://github.com/BuilderIO/qwik/blob/92b77bbe4dbbe623f26dcaf3165e3e1940b6b4af/packages/qwik/src/optimizer/src/optimizer.ts#L68

So it should be a single root file that encompasses all qwik code. Would be nicer as an array.

@mhevery
Copy link
Contributor

mhevery commented Oct 5, 2023

Would be nicer as an array.

😁 PR ?

@wmertens wmertens linked a pull request Nov 28, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants