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

compiler/tests: dynamic events: validate handler before executing #4105

Merged
merged 7 commits into from Dec 24, 2019
Merged

compiler/tests: dynamic events: validate handler before executing #4105

merged 7 commits into from Dec 24, 2019

Conversation

ghost
Copy link

@ghost ghost commented Dec 13, 2019

Fixes #4087, #4090.

Background
Dynamic events are callbacks that reside in a variable instead of directly pointing to a function and are emitted by the compiler when the variable is mutated or assigned in some way.

I noticed in #4087 that the dynamic event handler test case was throwing exceptions, but still passing.
Further investigation uncovered an issue where we were not properly handling exceptions in the run-time during testing. This led to filing #4090 after the root cause of the exception turned out to the lack of callback validation inside the proxy function used for dynamic events.

This pull request addresses both of these issues along with adding much more coverage for dynamic events.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests tests with npm test or yarn test)

@ghost
Copy link
Author

ghost commented Dec 19, 2019

@Conduitry,
I dropped the codegen tests because they aren't needed and broke after some other commits were added. I did leave the update to the existing test though.

let err = "";
window.addEventListener('error', (e) => {
e.preventDefault();
err = e.message;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
err = e.message;
err += e.message;

should concat all the error messages

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review, I'll push up changes after the holiday or feel free to push a commit sooner if needed.

@Conduitry Conduitry merged commit 8a59693 into sveltejs:master Dec 24, 2019
@ghost ghost deleted the issue/gh-4090 branch January 22, 2020 19:06
taylorzane pushed a commit to taylorzane/svelte that referenced this pull request Dec 17, 2020
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.

CI: event-handler-dynamic fails with exceptions
2 participants