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: Tests are not rendered properly after version > 4.6.0 #699

Closed
3 tasks done
Zorkann opened this issue May 11, 2023 · 2 comments · Fixed by #701
Closed
3 tasks done

bug: Tests are not rendered properly after version > 4.6.0 #699

Zorkann opened this issue May 11, 2023 · 2 comments · Fixed by #701
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. released

Comments

@Zorkann
Copy link

Zorkann commented May 11, 2023

Prerequisites

Liquid version

4.6.0 and above

Framework bindings

React

Current behavior

Looks like RTL cannot render liquid components properly. When we are using defineCustomElements() in tests it renders plain JSX element instead of HTML tag but only for some specific tags like <article> | <section> |<header>|<footer> etc. Before version 4.6.0 we did not use defineCustomElements() and everything was rendered properly. Because of that we are not able to generate snapshots and write tests for elements which are children of liquid elements.

  1. Using defineCustomElements() and using <article> = invalid
  2. Using defineCustomElements() and using <div> = valid
  3. Not using defineCustomElements() and using <article> = valid
  4. Not using defineCustomElements() and using <div> = valid
  5. Not using defineCustomElements() and using <LdCard><article></LdCard> = invalid // Was valid on previous versions
  6. Not using defineCustomElements() and using <LdCard><div></LdCard> = valid

So the behaviour is really weird and inconsistent.

Expected behavior

All components and html tags should be rendered properly in jest-dom.

Steps to reproduce

  1. Run tests in codesandbox and see different results in the console
  2. Follow the comments in the App.test.tsx and test all the cases

Code reproduction URL

https://codesandbox.io/p/github/emdgroup-liquid/liquid-sandbox-cra-tailwind/csb-lqyi2w/draft/fervent-curran?file=/src/App.test.tsx:15,4&selection=[{%22endColumn%22:52,%22endLineNumber%22:8,%22startColumn%22:52,%22startLineNumber%22:8}]

Additional information

No response

@Zorkann Zorkann added bug An issue describing unexpected or malicious behaviour. triage An issue that needs assessment to determine its validity and urgency labels May 11, 2023
@borisdiakur borisdiakur added confirmed This label indicates that the issue has been reproduced and verified by the core team. in progress This label indicates that the issue is currently being worked on. and removed triage An issue that needs assessment to determine its validity and urgency labels May 11, 2023
@borisdiakur
Copy link
Contributor

borisdiakur commented May 12, 2023

TODO:

  • Check if this is somehow related to Serializing anonymous classes of custom elements doesnt work with prettyDOM. testing-library/dom-testing-library#1191
    Nope, not related.

  • Check first line of code in node_modules/@emdgroup-liquid/liquid/dist/loader/index.js, which seems to be the root cause of the issue:

    // formatted first line of code
    (function () {
      if ("undefined" !== typeof window && void 0 !== window.Reflect && void 0 !== window.customElements) {
        var a = HTMLElement;
        window.HTMLElement = function () {
          return Reflect.construct(a, [], this.constructor)
        };
        HTMLElement.prototype = a.prototype;
        HTMLElement.prototype.constructor = HTMLElement;
        Object.setPrototypeOf(HTMLElement, a)
      }
    })();

    The purpose of this code appears to be to ensure that the HTMLElement constructor works correctly with the customElements API and can be used to define new custom elements in certain environments, which do not support this behavior out of the box. Most likely this is some ES5 related polyfill and we should find a way to get rid of it as it is only needed for old browsers (IE11 and below). It is already possible to import { defineCustomElements } from '@emdgroup-liquid/liquid/dist/loader/index.es2017.js' but it's kinda ugly to use this import. It should actually be used by default as the package.json within the loader is pointing to it. Maybe the actual problem is, that the react output target related code is importing the polyfilled loader...

borisdiakur added a commit that referenced this issue May 12, 2023
The ES5 polyfill included in the default loader messes with prettyDOM, which is used by testing-library. To fix this, we patch the loader package.json, the react and the vue output target imports to use the ES2017 code.

Fixes #699
borisdiakur added a commit that referenced this issue May 12, 2023
The ES5 polyfill included in the default loader messes with prettyDOM, which is used by testing-library. To fix this, we patch the loader package.json, the react and the vue output target imports to use the ES2017 code.

Fixes #699
github-actions bot pushed a commit that referenced this issue May 12, 2023
## [5.4.1](v5.4.0...v5.4.1) (2023-05-12)

### Bug Fixes

* **loader:** use ES2017 loader ([a079410](a079410)), closes [#699](#699)
@borisdiakur
Copy link
Contributor

🎉 This issue has been resolved in version 5.4.1 🎉

The release is available on:

📦🚀

@borisdiakur borisdiakur added released and removed in progress This label indicates that the issue is currently being worked on. labels May 12, 2023
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. released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants