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

improve variadic call using spread syntax #7030

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

cometkim
Copy link
Member

@cometkim cometkim commented Sep 10, 2024

@cometkim cometkim requested review from zth and cristianoc September 10, 2024 20:13
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

This is amazing.
Just one check: does this add any requirements to the version of JS required?
In case, we should document it.

@cristianoc
Copy link
Collaborator

CC @cknitt on the version constraints.
Can't remember if we have it documented somewhere, perhaps on the web page.

@cometkim
Copy link
Member Author

cometkim commented Sep 11, 2024

Applying by spread is ES6(ECMAScript 2015) spec, and has been supported longer than let in most browsers.

The only problem is IE11, which I don't think we support.

@cknitt
Copy link
Member

cknitt commented Sep 11, 2024

I don't think we have documented the requirements on the JS version / spec at the moment.

Starting with v12, ES6 (ECMAScript 2015) is required because we are now making use of the following ES6 features:

  • let keyword
  • arrow functions
  • spread operator

/cc @fhammerschmidt

@zth
Copy link
Collaborator

zth commented Sep 11, 2024

We should set a threshold for what syntax we support. But I think we should be fairly aggressive there so we can use newer features.

Copy link
Collaborator

@zth zth left a comment

Choose a reason for hiding this comment

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

Great stuff! Maybe all/parts of this could be implemented to use real array spreads instead of concats with the array spread syntax.

@cometkim
Copy link
Member Author

Great stuff! Maybe all/parts of this could be implemented to use real array spreads instead of concats with the array spread syntax.

Right. It could be reused in other places like the array and dict output

@zth
Copy link
Collaborator

zth commented Sep 11, 2024

@cometkim ready to merge?

@cometkim
Copy link
Member Author

Yep

@cometkim cometkim merged commit f3917c9 into rescript-lang:master Sep 12, 2024
20 checks passed
@cometkim cometkim deleted the variadic-call branch September 12, 2024 04:53
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.

4 participants