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

Transition internals #4999

Closed
wants to merge 12 commits into from
Closed

Conversation

pushkine
Copy link
Contributor

@pushkine pushkine commented Jun 10, 2020

Following #4742
Unfortunately crossfading and the animate directive turned out to be incompatible with transitions by design. There is still a few things related to transitions from that PR, none of them are breaking

notes

  • motion still uses the old loop system, I started rewriting some of it using motion stores from Transitions fix #4742 but I doubt anyone using the current implementation is concerned about performance
  • transitions could be DRYer, and bidi are heavy on GC
  • @rollup/plugin-typescript has to be updated to properly inline const enum

todo

Remove transition easing strategies ( cool but unnecessary )
Write tests for fixed bugs

Can't work on this for some time but it's there if someone needs it, it's all yours

@antony antony requested a review from Rich-Harris June 10, 2020 09:20
@pushkine
Copy link
Contributor Author

pushkine commented Jun 10, 2020

Also here's a playground for the fork https://svelte.dev/repl/3548e4d0a06f4440847e5c92caa02b93
Each column uses a different strategy ( the way easing is computed when transitions are reversed )
Changing easing does not work on the web repl since transitions are only ever computed once

always happy to instant chat on discord if you need me

Comment on lines +38 to +43
export const transition_out = (block: Fragment, local?) => {
// todo : are `!block` and `outroing.has` checks necessary ?
if (!block || !block.o || outroing.has(block)) return;
outroing.add(block);
block.o(local);
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rereading it this looks like a memory leak, should push outroing.delete(block) into a transition group callback

@stale
Copy link

stale bot commented Jun 26, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Jun 26, 2021
@stale stale bot removed the stale-bot label Jun 26, 2021
@pngwn pngwn added compiler Changes relating to the compiler stale-bot and removed feature: transitions temp-stale labels Jun 27, 2021
@stale stale bot removed the stale-bot label Jun 27, 2021
@benmccann
Copy link
Member

There's some really good stuff here, but it's also just not possible to review given the size of the PR. Can you split it up into multiple smaller PRs? We've been trying to clear out the review queue, so I'm going to go ahead and close this given that I don't see any path forward on it in its current state. That doesn't mean that we don't appreciate this work, but it's just acknowledging the reality that no PR this big can be reviewed, which is why it's sat here for so long. If you'd like to discuss ways of splitting up the PR you can find us on Discord in the #contribute-to-svelte channel

@benmccann benmccann closed this Jul 22, 2021
@pushkine pushkine deleted the transitions-rework branch July 23, 2021 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment