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(shadow-dom): prevent slot leak #678

Merged
merged 21 commits into from Jan 24, 2020
Merged

Conversation

@bigopon
Copy link
Member

bigopon commented Jan 22, 2020

Copied from #677


Fixes aurelia/templating-resources/issues/392

Thanks to @thomas-darling for this fix.


Currently, ShadowSlot keeps track of what Node has been projected through it, via an internal children array but it doesn't untrack(remove) those tracked Node from the array when the owning views of those Nodes get removed. Fix this by removing them in ShadowSlot.prototype.removeView and ShadowSlot.prototype.removeAll

Also revamp testing setup so that it can remove the usage of jspm for testing, while keeping the build process intact. Now we have:

npm run test
npm run test:watch
npm run test:debugger

with an option to go full integration tests to have more solid infra for tricky parth such as Shadow DOM emulation.

@EisenbergEffect @fkleuver @StrahilKazlachev

@bigopon bigopon mentioned this pull request Jan 22, 2020
@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Jan 24, 2020

Thank you @bigopon and @thomas-darling !

@thomas-darling were you able to confirm that this works well in your app without any breaks?
@bigopon Thanks for the extra work you put into improving things, besides the original fix. Is this all ready to merge?

@bigopon

This comment has been minimized.

Copy link
Member Author

bigopon commented Jan 24, 2020

Yes it is

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Jan 24, 2020

Ok, lets see if our dev branch build and merge CI works with the new stuff. Here goes...

@EisenbergEffect EisenbergEffect merged commit 9241dca into aurelia:develop Jan 24, 2020
4 checks passed
4 checks passed
WIP Ready for review
Details
ci/circleci: build_test Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details
main Workflow: main
Details
@thomas-darling

This comment has been minimized.

Copy link

thomas-darling commented Jan 24, 2020

@EisenbergEffect
So far it works perfectly in our app - nothing breaking, and it appears to have fixed the leaks :-)

Do you have an ETA for a release that includes this fix?
I have some other apps in production that, although less critical, could use this fix too :-)

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Jan 24, 2020

I should be able to get the release out within the next few days.

@bigopon bigopon deleted the bigopon:develop branch Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.