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.

Sign up for GitHub

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

Jump to bottom

Prevent harness code not loading when async/generator/asyncGenerator not supported #4164

Merged
merged 2 commits into from
Jul 24, 2024
Merged

Prevent harness code not loading when async/generator/asyncGenerator not supported #4164

merged 2 commits into from
Jul 24, 2024

Conversation

p-bakker
Copy link
Contributor

@p-bakker p-bakker commented Jul 23, 2024 •
edited
Loading

In order to not fail because harness code fails to load due to using not-supported EcmaScript features while not needed

Relates to ##3032

@p-bakker p-bakker requested a review from a team as a code owner July 23, 2024 16:06
@p-bakker p-bakker force-pushed the 3032_dont_throw_on_unsupported_hidden_constructor branch from a4f2d37 to 5969271 Compare July 23, 2024 16:19
@p-bakker p-bakker changed the title Dont throw when async/generato/asyncGenerator not supported Prevent harness code not loading when async/generaton/asyncGenerator not supported Jul 23, 2024
@p-bakker p-bakker changed the title Prevent harness code not loading when async/generaton/asyncGenerator not supported Prevent harness code not loading when async/generator/asyncGenerator not supported Jul 23, 2024
ptomato
ptomato requested changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I'm less sure about this one, because this harness file is only used in 4 tests which are all guaranteed to fail anyway if async or generators aren't supported. So doesn't really matter if they fail in the harness file.

Or is the problem that you get a SyntaxError instead of a runtime error? (In that case, could we use the new Function('...') without swallowing the exception with catch(e) {}? I tend not to like unconditionally dropping exceptions...)

harness/hidden-constructors.js Outdated Show resolved Hide resolved
harness/hidden-constructors.js Outdated Show resolved Hide resolved
@p-bakker
Copy link
Contributor Author

@ptomato the issue is any error: if for example only one of the three flavors isn't supported, that shouldn't stop/prevent the other two variables from being instantiated.

I don't see how that can be achieved without catching potential exceptions

@ptomato
Copy link
Contributor

ptomato commented Jul 24, 2024

Got it, thanks. I understand now.

@ptomato ptomato force-pushed the 3032_dont_throw_on_unsupported_hidden_constructor branch from 089878f to 8dea11c Compare July 24, 2024 16:12
@ptomato ptomato merged commit f55e997 into tc39:main Jul 24, 2024
8 checks passed
@anba anba mentioned this pull request Jul 24, 2024
Closed
ptomato added a commit to ptomato/test262 that referenced this pull request Jul 24, 2024
@ptomato
Fix syntax error from tc39#4164
368b39b 
Function statements require a name.

See tc39#4166
ptomato added a commit that referenced this pull request Jul 24, 2024
@ptomato @ljharb
Fix syntax error from #4164 (#4167)
50b023e 
* Fix syntax error from #4164

Function statements require a name.

See #4166

* Apply suggestions from code review

Co-authored-by: Jordan Harband <ljharb@gmail.com>

---------

Co-authored-by: Jordan Harband <ljharb@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

@ptomato ptomato ptomato approved these changes

Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@p-bakker @ptomato @ljharb