Skip to content

Handle external reassignments#2240

Merged
lukastaegert merged 2 commits intomasterfrom
handle-external-reassignments
Jun 4, 2018
Merged

Handle external reassignments#2240
lukastaegert merged 2 commits intomasterfrom
handle-external-reassignments

Conversation

@lukastaegert
Copy link
Member

Resolves #2237

As it turns out, there was nothing to do for objects attached to globals as the global variable handling has always been very conservative here. Added a regression test nonetheless.

External exports are now properly marked as reassigned.

@lukastaegert lukastaegert requested a review from guybedford June 4, 2018 09:35
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Looks great, and nice work on the quick fix. Just the one question, but good to merge.

const variable = this.traceExport(exportName);

variable.exportName = exportName;
variable.reassignPath(UNKNOWN_PATH, NEW_EXECUTION_PATH);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering, if we are going to support object truthiness in future like:

var obj = {};
if (!obj || typeof obj !== 'object')
  console.log('treeshaken');

Then would it make sense to have a form of reassignPath that can treat the object as still an object, but with unknown properties?

So the following optimization could still work:

const obj = {
	reassigned() {},
	test() {
		obj.reassigned();
	}
};

export default obj;

if (!obj)
  console.log('treeshaken');

Copy link
Member Author

Choose a reason for hiding this comment

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

This would be supported by the current approach. UNKNOWN_PATH might be a sub-optimal variable name as it is actually an "unknown path of length one". Thus the object is still recognized as an object but we no longer have any reliable information about the values of its fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, great to know.

@lukastaegert lukastaegert added this to the 0.60.0 milestone Jun 4, 2018
@lukastaegert lukastaegert merged commit a46fb0b into master Jun 4, 2018
@lukastaegert lukastaegert deleted the handle-external-reassignments branch June 5, 2018 07:10
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.

2 participants