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

Use redefining helper pattern for better tree-shakeability #24244

Merged
merged 2 commits into from Jun 18, 2018

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented May 18, 2018

  • Code is up-to-date with the master branch
  • You've successfully run jake runtests locally
  • You've signed the CLA

I'm not yet sure if I've made this change correctly, need to fix tests first - but this presents the idea, so let me know if you'd like to proceed with this or not. By changing extendStatics & assign to declarations they become effectively more static and thus it makes the whole thing more recognizable as side-effect free, so other tooling has easier job at tree-shaking them.

We use this pattern currently in babel. You can check here.

This PR fails at:

TypeError: Assignment to constant variable.

I suspect that those helpers are transformed to const somewhere, but I don't know where yet. I'd appreciate some hint about this one

@msftclas
Copy link

msftclas commented May 18, 2018

CLA assistant check
All CLA requirements met.

@j-oliveras
Copy link
Contributor

Your change is not valid. Running your __assign version with node 8.11.1 calling as __assign({}, {a: 1}) I get an RangeError: Maximum call stack size exceeded exception instead of { a: 1}.

@Andarist
Copy link
Contributor Author

You are right, I've reassigned wrong identifier - it should be fixed now.

@mhegazy mhegazy requested a review from rbuckton May 18, 2018 19:29
@mhegazy
Copy link
Contributor

mhegazy commented May 18, 2018

@rbuckton thoughts?

@rbuckton
Copy link
Member

@mhegazy - These changes look fine to me.

@Andarist - Where are you seeing "TypeError: Assignment to constant variable."? Is it from reassigning extendsStatics since it is declared as a function?

@Andarist Andarist force-pushed the redefining-helpers branch 4 times, most recently from 7c30d86 to 6a45805 Compare May 19, 2018 08:49
@Andarist
Copy link
Contributor Author

@Andarist - Where are you seeing "TypeError: Assignment to constant variable."? Is it from reassigning extendsStatics since it is declared as a function?

Not sure, but I was reassigning wrong identifier at first - I've fixed it and the problem is gone. However still the received error seemed to be wrong as my code error was causing runtime recursion error and not TypeError, but 🤷‍♂️

I've updated baseline tests, from my perspective it is ready to merge if you decide the change is worth it 😉

@j-oliveras
Copy link
Contributor

I think that the @rbuckton comment is because your current __extends at playground has a compile error at extendStatics assignment with message Cannot assign to 'extendStatics' because it is not a variable. The code is:

var __extends = (this && this.__extends) || (function () {
	function extendStatics(d, b) {
		extendStatics = Object.setPrototypeOf ||
			({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
			function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
		return extendStatics(d, b);
	}

	return function (d, b) {
		extendStatics(d, b);
		function __() { this.constructor = d; }
		d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
	};
})();

Seems it is valid to reassign a function in javascript but typescript don't allow it. I don't know if is by design.

How to change it to this?

var __extends = (this && this.__extends) || (function () {
    var extendStatics = function (d, b) {
        extendStatics = Object.setPrototypeOf ||
            ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
            function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
        return extendStatics(d, b);
    };

    return function (d, b) {
        extendStatics(d, b);
        function __() { this.constructor = d; }
        d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
    };
})();

@Andarist
Copy link
Contributor Author

Sure, I can change extendStatics to function expression if needed - just let me know.

@Andarist
Copy link
Contributor Author

@rbuckton friendly 🏓 is there anything left to do here? is this something that you would like to incorporate?

Copy link
Member

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

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

One minor change, per the earlier discussion.

Also, would you be interested in making this change in https://github.com/Microsoft/tslib as well?

var extendStatics = Object.setPrototypeOf ||
({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
function extendStatics(d, b) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you make this a var per #24244 (comment)?

@Andarist
Copy link
Contributor Author

One minor change, per the earlier discussion.

Done.

Also, would you be interested in making this change in https://github.com/Microsoft/tslib as well?

That was my original intention/reason why I've started this discussion here 😉 Is tslib manually updated or is it done with some script?

@mhegazy
Copy link
Contributor

mhegazy commented Jun 5, 2018

@rbuckton can u take one more look?

@Andarist
Copy link
Contributor Author

@rbuckton friendly 🏓 😉

@rbuckton
Copy link
Member

Sorry for the delay.

Is tslib manually updated or is it done with some script?

tslib is manually updated currently.

@rbuckton rbuckton merged commit aa26a59 into microsoft:master Jun 18, 2018
@Andarist Andarist deleted the redefining-helpers branch June 18, 2018 08:29
@Andarist
Copy link
Contributor Author

tslib is manually updated currently.

Ok, thanks for the info - gonna prepare a followup PR there.

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.

None yet

5 participants