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] tracking whether transition has started #6399

Merged
merged 1 commit into from
Jul 23, 2021

Conversation

almogtavor
Copy link
Contributor

@almogtavor almogtavor commented Jun 12, 2021

Tests

  • Run the tests with npm test and lint the project with npm run lint

@benmccann
Copy link
Member

Is there an issue in the tracker describing how this manifests or should there be a test for this?

@almogtavor
Copy link
Contributor Author

No there's no referring issue

@benmccann
Copy link
Member

Can you give some more details? The current code is clearly broken, but how do you trigger this and what's the symptom? E.g. I'm wondering if it's better to fix started as you're doing here or just remove it if it's unused and has no effect

@almogtavor
Copy link
Contributor Author

Well I think it's pretty hard to trigger such thing. The symptom isn't too harmful - just potentially calling the go function redundantly, because it has been already called. I don't think there's a reason to delete this function since it can hurt transition's internal functionalities, which seems pretty fine.

@almogtavor
Copy link
Contributor Author

@benmccann what do you think?

@benmccann
Copy link
Member

Can you add a test that fails without the change?

@benmccann benmccann changed the title Fix broken start() function that will always dispatch due to an always-false variable [fix] Fix broken start() function that will always dispatch due to an always-false variable Jun 30, 2021
@benmccann benmccann changed the title [fix] Fix broken start() function that will always dispatch due to an always-false variable [fix] tracking whether transition has started Jul 23, 2021
@benmccann benmccann merged commit 9d21aa1 into sveltejs:master Jul 23, 2021
@Conduitry
Copy link
Member

This has been released in 3.40.2.

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

Successfully merging this pull request may close these issues.

3 participants