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

bug: Can't resolve react-component-lib since v4.0.0 #495

Closed
3 tasks done
mkilp opened this issue Jan 30, 2023 · 14 comments · Fixed by #502
Closed
3 tasks done

bug: Can't resolve react-component-lib since v4.0.0 #495

mkilp opened this issue Jan 30, 2023 · 14 comments · Fixed by #502
Assignees
Labels
bug An issue describing unexpected or malicious behaviour. confirmed This label indicates that the issue has been reproduced and verified by the core team. has workaround A workaround exists to handle the issue until it is fixed. released upstream Used to label issues which require a fix upstream (fixing an issue of a dependency).

Comments

@mkilp
Copy link

mkilp commented Jan 30, 2023

Prerequisites

Liquid version

4.0.0 + (Tried 4.3.5 and 4.0.0)

Framework bindings

React

Current behavior

Cannot use Liquid anymore since the switch to ESM modules.
Breaks with following error:


ERROR in ./node_modules/@emdgroup-liquid/liquid/dist/react.js 6:0-61

Module not found: Error: Can't resolve './react-component-lib' in 'C:\Users\-\WebstormProjects\-\node_modules\@emdgroup-liquid\liquid\dist'
Did you mean 'index.js'?
BREAKING CHANGE: The request './react-component-lib' failed to resolve only because it was resolved as fully specified
(probably because the origin is strict EcmaScript Module, e. g. a module with javascript mimetype, a '*.mjs' file, or a '*.js' file where the package.json contains '"type": "module"').
The extension in the request is mandatory for it to be fully specified.
Add the extension to the request.

Expected behavior

Liquid imports as expected.

Steps to reproduce

Not able to reproduce in public sandbox.

Code reproduction URL

Not able to reproduce in public sandbox.

Additional information

Feel free to reach out for a private demo on MS Teams if bug is unknown to the team!

@mkilp mkilp added bug An issue describing unexpected or malicious behaviour. triage An issue that needs assessment to determine its validity and urgency labels Jan 30, 2023
@borisdiakur borisdiakur added needed: replication This label indicates that a bug has been reported, but has not been successfully replicated yet. upstream Used to label issues which require a fix upstream (fixing an issue of a dependency). has workaround A workaround exists to handle the issue until it is fixed. needed: more information This label indicates that a reply with more information is required from the issue reporter. and removed triage An issue that needs assessment to determine its validity and urgency needed: replication This label indicates that a bug has been reported, but has not been successfully replicated yet. labels Jan 31, 2023
@borisdiakur
Copy link
Contributor

borisdiakur commented Jan 31, 2023

Hi @mkilp 👋
Thanks for bringing this up. 🙏

There is an upstream issue which includes a workaround which you can use until the upstream issue is resolved / until we find a proper fix for this:

  1. Install patch-package as a dev dependency
  2. Fix all relative imports to include the '.js' file extension in ./node_modules/@emdgroup-liquid/liquid/dist/react.js and all js files inside the react-component-lib folder; for instance:
import { createReactComponent } from './react-component-lib/index.js';
  1. Run patch-package to create a .patch file.
npx patch-package @emdgroup-liquid/liquid
  1. Add the following postinstall script to your package.json scripts object: "postinstall": "patch-package"
  2. Commit the patch file and the changes to the package.json file to share the fix with your team
  3. Delete the .cache folder inside the node_modules directory.

I'm attaching my patch file here:

@emdgroup-liquid+liquid+4.4.1.patch

It would be great to have more information on your project setup in order to be able to reproduce the issue and see, if this is something that we can fix on our end or if we need to communicate this further to an upstream project with the additional information. Feel free to update your issue description with anything that may seem relevant (are you using Webpack? Which version? CRA? Next? Which version? etc. ).

@vvision
Copy link

vvision commented Jan 31, 2023

Hello,

Similar issue here when updating to liquid v4.3.5.

App is using CRA with version "react-scripts": "5.0.1".

ERROR in ./node_modules/@emdgroup-liquid/liquid/dist/components/fetchAsset.js 1:0-54
Module not found: Error: Can't resolve '@stencil/core/internal/client' in '/redacted_path/node_modules/@emdgroup-liquid/liquid/dist/components'
Did you mean 'index.js'?
BREAKING CHANGE: The request '@stencil/core/internal/client' failed to resolve only because it was resolved as fully specified
(probably because the origin is strict EcmaScript Module, e. g. a module with javascript mimetype, a '*.mjs' file, or a '*.js' file where the package.json contains '"type": "module"').
The extension in the request is mandatory for it to be fully specified.
Add the extension to the request.

@mkilp
Copy link
Author

mkilp commented Jan 31, 2023

Hi @mkilp 👋 Thanks for bringing this up. 🙏

There is an upstream issue which includes a workaround which you can use until the upstream issue is resolved / until we find a proper fix for this:

  1. Install patch-package as a dev dependency
  2. Fix the ./node_modules/@emdgroup-liquid/liquid/dist/react.js file by adding the .js file extension to the import:
import { createReactComponent } from './react-component-lib/index.js';
  1. Run patch-package to create a .patch file.
  2. Add the following postinstall script to your package.json scripts object: "postinstall": "patch-package"
  3. Commit the patch file and the changes to the package.json file to share the fix with your team

It would be great to have more information on your project setup in order to be able to reproduce the issue and see, if this is something that we can fix on our end or if we need to communicate this further to an upstream project with the additional information. Feel free to update your issue description with anything that may seem relevant (are you using Webpack? Which version? CRA? Next? Which version? etc. ).

We are using CRA, with JS and react-scripts 5.0.1.

Thanks for the help, I will take a look at the workaround!

@borisdiakur borisdiakur removed the needed: more information This label indicates that a reply with more information is required from the issue reporter. label Jan 31, 2023
@borisdiakur borisdiakur self-assigned this Jan 31, 2023
@borisdiakur borisdiakur added the in progress This label indicates that the issue is currently being worked on. label Jan 31, 2023
@mkilp
Copy link
Author

mkilp commented Jan 31, 2023

@borisdiakur the workaround sadly does not work at all and creates a whole bunch of downstream problems.
It seems like all downstream files have the same issue:

ERROR in ./node_modules/@emdgroup-liquid/liquid/dist/react-component-lib/createComponent.js 20:0-120
Module not found: Error: Can't resolve './utils' in 'C:\Users\-\WebstormProjects\-\node_modules\@emdgroup-liquid\liquid\dist\react-component-lib'
Did you mean 'index.js'?

I think there might be some incompatibilities with react-scripts 5.x. I will try downgrading to 4.x and see if that helps.

@mkilp
Copy link
Author

mkilp commented Jan 31, 2023

Update: Downgrading react-scripts to 4.0.3 fixes the issue. I assume that means your problem has to do with webpack 5. I hope it can be resolved soon!

@borisdiakur
Copy link
Contributor

@mkilp This is a good hint. Setting resolve.fullySpecified in the webpack config to false might resolve the issue.

@mkilp
Copy link
Author

mkilp commented Jan 31, 2023

That might help. For now we will use the downgraded react-scripts as we don't want to eject our cra config. Hopefully the upstream issue gets resolved soon - not sure why I wasn't able to reproduce in a blank sandbox with only cra 5.0.1 and liquid.

@borisdiakur borisdiakur added the confirmed This label indicates that the issue has been reproduced and verified by the core team. label Jan 31, 2023
@borisdiakur
Copy link
Contributor

I have created a sandbox app with CRA and liquid:
https://github.com/emdgroup-liquid/liquid-sandbox-cra-tailwind
https://codesandbox.io/p/github/emdgroup-liquid/liquid-sandbox-cra-tailwind/main
It contains the patch as a workaround for the given issue.
It took me quite some time to make Jest tests run, but that's also working now.
I'll try to create a PR to the stencil-ds-output-targets project tomorrow.

@borisdiakur
Copy link
Contributor

borisdiakur commented Feb 3, 2023

I have created a pull request to the stencil-ds-output-targets project. Let's hope someone from the Stencil team will pick it up soon. In the meantime I'll try to create a patch for @emdgroup-liquid/liquid, so that you do not need to apply any workarounds in your own projects.

@borisdiakur
Copy link
Contributor

🎉 This issue has been resolved in version 4.4.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@borisdiakur borisdiakur added released and removed in progress This label indicates that the issue is currently being worked on. labels Feb 6, 2023
@vvision
Copy link

vvision commented Feb 7, 2023

Hello,

Upgrading to v4.4.3 does not fix the issue in our repository.
Looking at the content of the pull request, I understand the fix as yarn only.

I have created a minimal app to reproduce the issue.
It is available here: vvision/bug-liquid-react.

Maybe it works for the original issue. If needed, I can open a new issue.
Lets hope we will not have to wait too long to get ionic-team/stencil-ds-output-targets#322 to be merged.

ERROR in ./node_modules/@emdgroup-liquid/liquid/dist/components/ld-sr-only2.js 1:0-89
Module not found: Error: Can't resolve '@stencil/core/internal/client' in '/redacted/my-app/node_modules/@emdgroup-liquid/liquid/dist/components'
Did you mean 'index.js'?
BREAKING CHANGE: The request '@stencil/core/internal/client' failed to resolve only because it was resolved as fully specified
(probably because the origin is strict EcmaScript Module, e. g. a module with javascript mimetype, a '*.mjs' file, or a '*.js' file where the package.json contains '"type": "module"').
The extension in the request is mandatory for it to be fully specified.
Add the extension to the request.

Edit:
Seems related to the way we were doing our import with liquid v2: https://github.com/vvision/bug-liquid-react/blob/master/src/liquid/index.ts.

Switching to

import { defineCustomElements } from '@emdgroup-liquid/liquid/dist/loader';
defineCustomElements();

when using liquid v4.4.3, is fixing the issue.

@borisdiakur
Copy link
Contributor

Hi @vvision,
you should use the React bindings in your React app. Then it'll work. Here is what you need to change in your project:

  1. Remove all liquid component imports in src/liquid/index.ts. Instead add the following:
// use liquid.global.css if you are not using CSS components. It's way smaller than liquid.css
import "@emdgroup-liquid/liquid/dist/css/liquid.global.css";
import { defineCustomElements } from '@emdgroup-liquid/liquid/dist/loader'

// @ts-ignore
window.__LD_ASSET_PATH__ = `${window.location.origin}/liquid`

defineCustomElements() // this line is important for the components to work
  1. Now you can import React components from @emdgroup-liquid/liquid/dist/react. In your src/App.tsx you would have the following code:
import './App.css';
import './liquid';

import {
  LdTypo,
  LdButton,
} from '@emdgroup-liquid/liquid/dist/react'

function App() {
  return (
    <div className="App">
      <LdTypo variant="b2">
        App Title
      </LdTypo>
      <br/>
      <LdButton size="sm">A button</LdButton>
    </div>
  );
}

export default App;

I strongly recommend using the React bindings, because you'll benefit from type checking and intellisense. And of course, your issue will be resolved.

You can have a look at our create-react-app sandbox application for a complete working example.

@vvision
Copy link

vvision commented Feb 9, 2023

Thanks for the insights!

Using React bindings indeed fixes the issue, but the automatic lazy loading introduces very surprising behavior, with our main centered content being displayed first, then the left menu and finally, the footer and the header.

I'm also seeing similar behavior on https://github.com/emdgroup-liquid/liquid-sandbox-cra-tailwind, when throttling network to "Slow 3G". Which I assume is normal because of lazy loading.

I read about custom elements in Stencil doc to assess liquid dist-custom-elements.
I'm seeing the same issue as initially when importing component with react-scripts 5.0.1 (Works with 4.0.3) in my test repository:

import "@emdgroup-liquid/liquid/dist/css/liquid.global.css";
import { defineCustomElement as defineButton } from '@emdgroup-liquid/liquid/dist/components/ld-button';
defineButton();

@borisdiakur
Copy link
Contributor

Hi @vvision
usually you can have control over how custom elements are defined in your app. You could do something like this:

import { LdButton as LdButtonCE } from '@emdgroup-liquid/liquid/dist/components/ld-button'
import { LdButton } from '@emdgroup-liquid/liquid/dist/react'

// @ts-ignore
customElements.get('ld-button') ||
  customElements.define('ld-button', LdButtonCE)

However, due to how CRA handles imports from ESM modules this is currently not working. It would also involve a lot of patching of stencil code, just to fix quirks on CRA side. What you could do is, you could adjust your Webpack config without ejecting using craco (see this issue for reference). However, I would rather suggest to switch over to using Vite and forget about CRA. Using Vite solves all the issues listed above. You can have a look at our Vite based react sandbox app, if you'd like to take Vite for a spin. Otherwise, if you must stay with CRA and feel that the current solution with lazy loading doesn't work for you, I'd like to ask of you to open up a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue describing unexpected or malicious behaviour. confirmed This label indicates that the issue has been reproduced and verified by the core team. has workaround A workaround exists to handle the issue until it is fixed. released upstream Used to label issues which require a fix upstream (fixing an issue of a dependency).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants