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

feat(css-resource): add configuration option to remove injected styles #344

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@fkleuver
Copy link
Member

fkleuver commented Mar 27, 2018

This feature is requested now and then, and I'd also like to have it myself. It's probably not the cleanest solution, but I've tested it locally with webpack and works fine.

It's opt-in and will not change anything for existing applications.

Usage requires manually configuring aurelia and passing in the option, like so:

au.use
  .defaultBindingLanguage()
  .history()
  .router()
  .eventAggregator();
au.use.plugin(PLATFORM.moduleName('aurelia-templating-resources'), (opts: any) => {
  opts.removeInjectedStylesOnBeforeUnbind = true;
});

What this option will do is register the ViewEngineHooks (which normally only happens for as="scoped"), keep track of the injected StyleNode and remove it on beforeUnbind.

Since removing it will actually keep it removed across re-visits, it will add it again on beforeBind in case it hasn't been added yet by beforeCompile.

I also spotted a bug in the existing code: } else if (this._global && !this.owner._alreadyGloballyInjected). It's possible that this prevented as="scoped" from working correctly. I changed this._global to this.owner._global which should fix that as well.

@fkleuver fkleuver force-pushed the fkleuver:master branch from c0fa3ab to 633b8f3 Mar 27, 2018

@Alexander-Taran

This comment has been minimized.

Copy link
Member

Alexander-Taran commented Mar 27, 2018

package.lock.json intentional?

@fkleuver fkleuver force-pushed the fkleuver:master branch from 633b8f3 to 4ee7984 Mar 27, 2018

@fkleuver

This comment has been minimized.

Copy link
Member Author

fkleuver commented Mar 27, 2018

Most certainly isn't, fixed it. Thanks :)

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Mar 29, 2018

@fkleuver This looks great! I was planning to build this into vNext too :) Before I merge the PR and do a release, can you write some documentation?

@fkleuver

This comment has been minimized.

Copy link
Member Author

fkleuver commented Mar 29, 2018

Sure, though I'm not sure where to put that documentation. The readme doesn't seem like the right place for it at least.. or do you mean comments?

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Mar 29, 2018

Take a look in the templating repo. That's where most of the docs are for this sort of thing. It's a bit odd, I know.

@fkleuver

This comment has been minimized.

Copy link
Member Author

fkleuver commented Mar 29, 2018

@EisenbergEffect Added a short description, see last commit. Would that suffice?

@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Mar 30, 2018

Will this affects elements with multiple instances ? Unbinding one of them will cause the style to be removed for the rest ?

@fkleuver

This comment has been minimized.

Copy link
Member Author

fkleuver commented Mar 30, 2018

Elements will only track the nodes they added themselves, so if 10 of the same are rendered then the 1st element has added the style; only if that element is unbound, the style will be removed ("for all of them", yes). The other 9 have no influence in this regard.

So arguably you'd want to keep track of all instances of an element, and only remove the style if all of them are gone. It's possible but a lot of work, and probably not worth it. It's probably worth a mention in the docs though.

@Alexander-Taran

This comment has been minimized.

Copy link
Member

Alexander-Taran commented Mar 30, 2018

Ouch.. exactly what I was asking you..
So basically I can't opt into it.
Because I <require> syles for every custom element
so any repeater would brake styling of the repeated elements on removal of 1st element?

If merged - it should be not mentioned anywhwere.
No instructions whatsoever on that there is a possibility like this.
No hints even.

Maybe.. a comment in source:
// UNSUPPORTED METHOD OF CSS REMOVAL FOR FKLEUVER.. COS HE'S SO COOL (-:
// USAGE IS PROHIBITED

yet better make that more cryptic

It will be misused. And will only lead to issues and feature requests.
Nah.. nobody will see it as a feature request.. Straight out bug report that'd be (-:

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Mar 30, 2018

I missed that detail. I don't think we want to merge something that would remove styles and break components. A simple instance counter might make it work...

@fkleuver

This comment has been minimized.

Copy link
Member Author

fkleuver commented Mar 30, 2018

@Alexander-Taran Forgive me, I was a bit tired when I wrote that. You're right this is silly :)

I'll see if I can fix this properly.

@Alexander-Taran

This comment has been minimized.

Copy link
Member

Alexander-Taran commented Mar 30, 2018

@fkleuver nothing to apologize for. That's what code review is for. (-:

@fkleuver

This comment has been minimized.

Copy link
Member Author

fkleuver commented Apr 8, 2018

The more I look into this the less I understand how this somehow appeared to work in a small project of mine. Probably something to do with the lazy loading of routes.

In webpack, all imported custom elements (either via require tags or globalResources()) have their styles globally injected even if they are not used anywhere.

That in itself doesn't really make sense to me. Css for an element should only be added when that element is added. If adding an element is not what triggers adding the css, then removing an element cannot / should not trigger removing the css either.

Webpack again....

So apart from this particular PR, I feel that this apparently eager loading of all custom element css is an issue in itself that needs to be solved. @EisenbergEffect do you agree with that or am I seeing this the wrong way?

@fkleuver

This comment has been minimized.

Copy link
Member Author

fkleuver commented Apr 26, 2018

To make this work, style injection needs to be changed at a fairly fundamental level. I'm closing this PR because I wrote this code based on a wrong understanding of how style injection currently works, and it won't work like this. I might have another crack at it at a later time.

@fkleuver fkleuver closed this Apr 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment