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

Minify more type constructors #45

Merged
merged 3 commits into from
Jul 12, 2016
Merged

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Jul 10, 2016

This thing is awesome: https://astexplorer.net/

@fregante fregante mentioned this pull request Jul 10, 2016
4 tasks
(function (Boolean, String, Number) {
return Boolean(a), String(b), Number(c);
})(MyBoolean, MyString, MyNumber);
(function (Boolean, String, Number, Object) {
Copy link
Member

Choose a reason for hiding this comment

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

What about Array?

Copy link
Member

Choose a reason for hiding this comment

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

I see that you're testing for local binding so we should just add it here to the test.

Copy link
Contributor Author

@fregante fregante Jul 10, 2016

Choose a reason for hiding this comment

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

Yes, missed that. I squashed the commits in a sensible way and added Array there.

@fregante fregante force-pushed the minify-more-type-constructors branch 2 times, most recently from 0172071 to 31891f6 Compare July 10, 2016 08:44
@fregante fregante force-pushed the minify-more-type-constructors branch from e8016c3 to e5a2d37 Compare July 10, 2016 12:29
expect(transform(source)).toBe(expected);
});

it("should turn Array(number) to [,] only if number is <=6", () => {
Copy link
Contributor Author

@fregante fregante Jul 10, 2016

Choose a reason for hiding this comment

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

The only issue here is oldIE: [,].length is 2 in IE8- and 1 everywhere else.

Specifically, none of these work correctly there:

Array(1);
Array(2);
Array(3);
Array(4);
Array(5);
Array(6);

Are you going to support IE8 anywhere else?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it's worth supporting IE8. But we need to at least mention this in the docs.

Copy link
Member

Choose a reason for hiding this comment

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

I'd vote we document it for now, we could always have a separate transform to run through and pick out stuff like this that IE would choke on.

@kangax kangax merged commit d627c33 into master Jul 12, 2016
@kangax
Copy link
Member

kangax commented Jul 12, 2016

We need to also figure out how to denote plugins as safe (always identical functionality) or unsafe (identical functionality in most cases but not always). The Array => [] and all that would have to be marked as unsafe transformations.

@hzoo hzoo deleted the minify-more-type-constructors branch July 12, 2016 22:15
@fregante
Copy link
Contributor Author

Unsafe in IE8- only, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants