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

Move style gathering from class finalization to initialization #866

Merged
merged 5 commits into from
Jan 17, 2020

Conversation

dfreedm
Copy link
Member

@dfreedm dfreedm commented Dec 20, 2019

By moving style gathering to initialization, stylesheets will only be
created for elements that have booted, and not for elements that have
only be registered.

This should be performance positive for applications with less than
ideal loading behaviors.

Fixes #870

@dfreedm
Copy link
Member Author

dfreedm commented Dec 20, 2019

Also fixed a few formatting issues.

CHANGELOG.md Outdated Show resolved Hide resolved
@web-padawan
Copy link
Contributor

IMO, this is a breaking change. As an example, we are using finalize in VaadinThemableMixin to gather styles once per class.

Would it be possible to keep protected static finalize() as empty method?

@dfreedm @justinfagnani WDYT?

@justinfagnani
Copy link
Contributor

@web-padawan finalize() still exists on UpdatingElement, so unless you're doing a hasOwnProperty check on LitElement, this should be fine.

@web-padawan
Copy link
Contributor

web-padawan commented Dec 20, 2019

@justinfagnani thanks for pointing out.

FWIW, we currently rely on _styles private property being set on finalized.
And we use this ugly hack to inject styles from dom-module.

  static _includeStyle(moduleName: string) {
    // Hack to bypass TypeScript private property check
    // eslint-disable-next-line dot-notation
    this['_styles'].push(unsafeCSS(cssFromModule(moduleName)));
  }

So I will have to adjust that to use initialize() instead and add own flag to only do it once.

But then again, we can't modify _styles anymore in a way that we are doing it 😕

Can you consider the following changes in scope of this PR?

  1. A point where we could inject our own styles:
  • make _initializeStyles method protected
  • make _getUniqueStyles method protected

Either option should work for our case.

  1. Optional: make _styles property protected (or expose API for one time modification)

@justinfagnani
Copy link
Contributor

@web-padawan this is the code in question, yeah? https://github.com/vaadin/component-mixins/blob/f0e0f1953a2cd2d937d91ca7311e34cf784157f4/packages/themable-element/themable-element.ts#L31

Taking a look now....

@dfreedm
Copy link
Member Author

dfreedm commented Dec 20, 2019

Also, @aomarks do you know why the Tachometer tests are stuck?

@aomarks
Copy link
Member

aomarks commented Dec 20, 2019

Also, @aomarks do you know why the Tachometer tests are stuck?

{ SessionNotCreatedError: session not created: This version of ChromeDriver only supports Chrome version 77
    at Object.throwDecodedError (/home/travis/build/Polymer/lit-element/node_modules/selenium-webdriver/lib/error.js:550:15)
    at parseHttpResponse (/home/travis/build/Polymer/lit-element/node_modules/selenium-webdriver/lib/http.js:563:13)
    at Executor.execute (/home/travis/build/Polymer/lit-element/node_modules/selenium-webdriver/lib/http.js:489:26)
    at process._tickCallback (internal/process/next_tick.js:68:7)
  name: 'SessionNotCreatedError',
  remoteStacktrace: '#0 0x55a27cdc9959 <unknown>\n' }
``

https://travis-ci.org/Polymer/lit-element/jobs/627497662

Looks like we need to bump the package-lock to get a new Chromedriver. I can send a PR.

@aomarks
Copy link
Member

aomarks commented Dec 21, 2019

Also, @aomarks do you know why the Tachometer tests are stuck?

Should be fixed in #868

By moving style gathering to initialization, stylesheets will only be
created for elements that have booted, and not for elements that have
only be registered.

This should be performance positive for applications with less than
ideal loading behaviors.
@dfreedm
Copy link
Member Author

dfreedm commented Dec 21, 2019

Rebased with #868

@dfreedm dfreedm mentioned this pull request Dec 23, 2019
@dfreedm dfreedm self-assigned this Jan 15, 2020
Override `getStyles()` to integrate a style injection system.
@dfreedm
Copy link
Member Author

dfreedm commented Jan 16, 2020

Apologies for the delay, dropped the ball on this after the Holiday break.

Copy link
Contributor

@justinfagnani justinfagnani left a comment

Choose a reason for hiding this comment

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

Still LGTM!

@web-padawan hows this look to you? You'd override getStyles().

Copy link
Contributor

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

### Fixed
* Properties annotated with the `eventOptions` decorator will now survive property renaming optimizations when used with tsickle and Closure JS Compiler.
* Moved style gathering from `finalize` to `initialize` to be more lazy, and create stylesheets on the first instance initializing [#866](https://github.com/Polymer/lit-element/pull/866).
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it makes sense to add statis getStyles() mention to "Added"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, will do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hook for Theming
5 participants