Skip to content

allow BindingPattern in FunctionRestParameter #26017

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

Merged
merged 8 commits into from
Jan 14, 2019

Conversation

ajafff
Copy link
Contributor

@ajafff ajafff commented Jul 27, 2018

Second part of #6275

Also adds downlevel emit for the destructured rest param.
I don't know if my change emits correct sourcemaps. @weswigham how do you test this stuff?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jul 30, 2018

Minor suggestion: can you put the tests in one file? Seems like they all could be there.

how do you test this stuff?

You can add a comment at the top of the file like

// @sourcemap: true

@ajafff
Copy link
Contributor Author

ajafff commented Jul 30, 2018

I merged the new tests into one file and added baselines for sourceMap generation. I don't fully understand these baselines, but they don't look too wrong to me 😄

@mhegazy mhegazy requested a review from weswigham July 30, 2018 18:47
@mhegazy
Copy link
Contributor

mhegazy commented Jul 30, 2018

@weswigham can you please review and merge

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

I'm blind, object binding tests are there: Could you enable sourcemaps on the one with object binding patterns, too? Also tests for initializers and type errors for object binding patterns, too.

@ajafff
Copy link
Contributor Author

ajafff commented Aug 10, 2018

@weswigham can you please review again? I updated the PR according to your comment a while ago.

@RyanCavanaugh RyanCavanaugh added this to the Community milestone Sep 17, 2018
@ChiriVulpes
Copy link
Contributor

Any news on this? I just ran into my first use case for it; now that there's nice tuple support I have some type aliases for tuples, it'd be nice to replace the function arguments with the existing tuple so it's updated if the tuple definition ever changes.

This would allow a method to be able to take a tuple or each value of the tuple easily, since if you're passing a tuple you just use the spread operator, whereas if you're passing the individual values it just works as is. Prevents having to construct new tuples with tuple = <T extends any[]>(...items: T) => items;

If you're curious, here's my Real World Code ™

type HighlightSelector = [HighlightType, string | number];

// in a class
public register(component: IComponent, selector: HighlightSelector, until = ComponentEvent.Remove) {
	const selectorId = this.getHighlightSelectorId(...selector);
	this.highlightComponents.set(selectorId, component);
	component.once(until, () => this.highlightComponents.delete(selectorId));
}

private getHighlightSelectorId(...[type, selector]: HighlightSelector) {
	return `${type}:${selector}`;
}

private getHighlightComponent([type, selector]: HighlightSelector) {
	if (type === HighlightType.Selector) {
		return Component.get(`${selector}`);
	}
	
	return this.highlightComponents.get(this.getHighlightSelectorId(type, selector));
}

@@ -29969,10 +29969,6 @@ namespace ts {
checkGrammarForDisallowedTrailingComma(parameters, Diagnostics.A_rest_parameter_or_binding_pattern_may_not_have_a_trailing_comma);
}

if (isBindingPattern(parameter.name)) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this error still be there if its a object binding pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean something like (...{length}) => length? The spec allows it and there are already tests to ensure type inference and downlevel emit works as expected.

@sheetalkamat sheetalkamat merged commit d4055a3 into microsoft:master Jan 14, 2019
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.

7 participants