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

fix(dist-custom-elements): add ssr checks #3131

Merged
merged 7 commits into from
Nov 9, 2021

Conversation

willmartian
Copy link
Contributor

@willmartian willmartian commented Nov 3, 2021

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Unit tests (npm test) were run locally and passed
  • E2E Tests (npm run test.karma.prod) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

GitHub Issue Number: #3101

What is the new behavior?

  • The generated defineCustomElement function now checks if customElements is defined.
  • Components in the custom elements build now check if HTMLElement is defined before extending from it:
const MyElement extends from (
  typeof HTMLElement !== 'undefined'
  ? HTMLElement
  : class {});

Does this introduce a breaking change?

  • Yes
  • No

Testing

Other information

This fix is a prerequisite for ionic-team/ionic-framework#24163

@willmartian willmartian requested a review from a team November 3, 2021 17:54
@willmartian willmartian marked this pull request as draft November 3, 2021 17:55
Copy link
Member

@rwaskiewicz rwaskiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I think these tests are failing is rooted in a declaration ordering error. If you npm ci && npm run build && pushd test/karma && npm run karma.prod, we see the tests fail (but that much you knew 🙃)

After doing that, open up www/custom-elements-delegates-focus/main.js (from the test/karma dir) and you'll see:

let CustomElementsDelegatesFocus$1 = class extends HTMLElementSSR {
  constructor() {
    super();
    this.__registerHost();
    this.__attachShadow();
  }
  render() {
    return (h(Host, null, h("input", null)));
  }
  static get delegatesFocus() { return true; }
  static get style() { return sharedDelegatesFocusCss; }
};
var HTMLElementSSR = (typeof HTMLElement !== "undefined" ? HTMLElement : class  {
});

If I reorder those two statements:

+ var HTMLElementSSR = (typeof HTMLElement !== "undefined" ? HTMLElement : class  {
+ });
let CustomElementsDelegatesFocus$1 = class extends HTMLElementSSR {
  constructor() {
    super();
    this.__registerHost();
    this.__attachShadow();
  }
  render() {
    return (h(Host, null, h("input", null)));
  }
  static get delegatesFocus() { return true; }
  static get style() { return sharedDelegatesFocusCss; }
};
- var HTMLElementSSR = (typeof HTMLElement !== "undefined" ? HTMLElement : class  {
- });

that test passes (npm run karma from within test/karma to avoid a rebuild), so it looks like that aspect of the PR at least is rooted in these types of ordering issues.

I haven't given this PR too much other focus, but I think that might get you going again 🏃

@willmartian
Copy link
Contributor Author

willmartian commented Nov 5, 2021

Update: This is a separate issue, not concerned with this PR. Created new GH issue here: #3134

Hi @rwaskiewicz, when I test this build in Ionic Framework, I am receiving this error in our Vue e2e tests: Uncaught DOMException: Failed to construct 'CustomElement': The result must not have attributes.

The same error is described here: https://stackoverflow.com/questions/43836886/failed-to-construct-customelement-error-when-javascript-file-is-placed-in-head

I don't believe this error is being caused by any of the changes in this PR, but this PR fixes earlier points of failure in those same e2e tests (customElements is not defined). So I am not getting to this error in our tests unless testing this PR's build.

The stackoverflow link above expands on the error, stating that it originates from accessing attributes or children in the custom element's constructor, which should not happen as per spec: https://html.spec.whatwg.org/multipage/custom-elements.html#custom-element-conformance

I am still familiarizing myself with the Stencil source and so I need some assistance verifying, do we follow the spec regarding the above?

I see that the generated custom element for ion-button for example, makes calls to this.__registerHost and this.__attachShadow. Do either of those functions potentially access attributes/children in the constructor?

@willmartian willmartian marked this pull request as ready for review November 8, 2021 15:26
@rwaskiewicz rwaskiewicz changed the title feat(dist-custom-elements): add SSR checks fix(dist-custom-elements): add SSR checks Nov 9, 2021
@rwaskiewicz rwaskiewicz changed the title fix(dist-custom-elements): add SSR checks fix(dist-custom-elements): add ssr checks Nov 9, 2021
@rwaskiewicz rwaskiewicz merged commit 9a232ea into ionic-team:main Nov 9, 2021
@rwaskiewicz rwaskiewicz added the Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. label Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants