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

FEAT: Support default parameters #1640

Merged
merged 1 commit into from
Sep 22, 2024
Merged

Conversation

0xe
Copy link
Contributor

@0xe 0xe commented Sep 21, 2024

Adds support for default values in function parameters and default values in destructuring assignment.

What's not implemented yet, that I plan to implement subsequently:

  • Destructuring with defaults in for loops (eg: for (let {x = 2} = {}; ; ))
  • Reading [Symbol.iterator] from default array/object

(Added ignored tests for these in DefaultParametersTest)

(same changes as #1481, but squashed and on top of master)

@gbrail
Copy link
Collaborator

gbrail commented Sep 22, 2024

Thanks -- this has been outstanding for a long time, and it makes a lot more things work so I'm going to merge it.

I notice that there are test262 tests that actually pass but are marked as failing, so I'm going to fix that and manually merge. Thanks for working on this!

@gbrail gbrail merged commit 9ca1469 into mozilla:master Sep 22, 2024
3 checks passed
@gbrail
Copy link
Collaborator

gbrail commented Sep 22, 2024

Merged manually with some test262 fixes -- looking forward to more!

@0xe
Copy link
Contributor Author

0xe commented Sep 22, 2024

Merged manually with some test262 fixes -- looking forward to more!

Thank you!

@p-bakker
Copy link
Collaborator

@0xe would you say we can close #756 now as well?

Unless you'll open a PR for the three things you mentioned (that aren't supported yet) very shortly, cloud you maybe create a follow-up issue, so we don't lose track of it?

Just checking: this PR didn't move the needle on rest/spread syntax anywhere, correct?

@0xe
Copy link
Contributor Author

0xe commented Sep 22, 2024

@0xe would you say we can close #756 now as well?

Unless you'll open a PR for the three things you mentioned (that aren't supported yet) very shortly, cloud you maybe create a follow-up issue, so we don't lose track of it?

Just checking: this PR didn't move the needle on rest/spread syntax anywhere, correct?

Yeah, I will create issues for the TODO items from this PR tonight.

There should be no changes to rest/spread from this PR.

For #756, I think the basic things should work. Yeah, I do think it makes sense to close that and perhaps create issues for things which are still broken.

@p-bakker
Copy link
Collaborator

Any chance you can have a look what, if anything, is still not working wrt default values in destructuring assignments?

I would think support/issues between destructuring assignments and default function parameter values to be quite similar, but I might be wrong.

For now I'm holding of on closing #756 a bit until we have some (more) insight into this

@Osiris-Team
Copy link

@0xe Wow amazing, 1k lines changed. Thanks for your work! I remember opening the issue 4 years ago as a complete Java noob. Time flies.

@0xe 0xe deleted the scratch/satish/sda branch September 22, 2024 14:41
@0xe
Copy link
Contributor Author

0xe commented Sep 22, 2024

Added an issue for known unimplemented things from this PR: #1641

@0xe
Copy link
Contributor Author

0xe commented Sep 22, 2024

Any chance you can have a look what, if anything, is still not working wrt default values in destructuring assignments?

I would think support/issues between destructuring assignments and default function parameter values to be quite similar, but I might be wrong.

For now I'm holding of on closing #756 a bit until we have some (more) insight into this

Makes sense. I will take a look at what's pending with destructuring assignments and update #756.

From my quick check in the shell, following expressions evaluate correctly:

js> let [a, b] = [1, 2];
js> a
1
js> b
2
js> let [c=3, d=4] = [];
js> c
3
js> d
4
js> let {e = 5, f = 6} = {};
js> e
5
js> f
6
js> let {g = 7, h = 8} = {g: 9, h: 10}
js> g
9
js> h
10

One thing that should be addressed in #1641 is reading [Symbol.iterator] when destructuring default values from objects.

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