Disable scope hoisting when 'this' points to an export #9291
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
October 4, 2023 16:57
Co-authored-by: Eric Eldredge <lettertwo@gmail.com> Co-authored-by: Brian Tedder <btedder@atlassian.com>
Benchmark ResultsKitchen Sink ✅
Timings
Cold Bundles
Cached Bundles
React HackerNews ✅
Timings
Cold Bundles
Cached Bundles
AtlasKit Editor ✅
Timings
Cold Bundles
Cached Bundles
Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. |
|
This does wrap unnecessarily on... class Foo {
constructor() {
this.a = 4
}
bar() {
return this.baz()
}
baz() {
return this.a
}
}
exports.baz = new Foo()
exports.a = 2Should we only track the |
adelchan07
requested review from
mischnic,
devongovett,
lettertwo and
imbrian
and removed request for
mischnic
mischnic
reviewed
Oct 6, 2023
mischnic
reviewed
Oct 6, 2023
Co-authored-by: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com>
|
I assume you ran into some specific dependency that uses this pattern just like #7866 did? Because this only fixes a very specific set of cases and not for example exports.foo = function () {
let x = this;
x.bar();
// or
let key = "bar";
this[key]();
};
exports.bar = function () {
return 2;
};But on the other hand, just testing for |
mischnic
reviewed
Oct 6, 2023
added 2 commits
October 6, 2023 15:14
This reverts commit a017082.
the PR was originally created to specifically address the issue with Would you prefer handling all cases related to 'this' pointing to an export in a single PR, or could we merge the fix for retry and handle other cases in another PR. From our side, would prefer to merge this PR in time for the next release to unblock another product that were integrating with Parcel. If we decide to split into different PRs, will update the PR description accordingly @imbrian @mischnic |
Ideally we'd have to do as little of this inexact this-detection as possible. So if you don't have to, don't 😅 The only concern I have with this solution is that you can get false positives, e.g. with exports.foo = function(){...};
// Usage: `new SomeClass()`
exports.SomeClass = function SomeClass(){
this.foo = 123;
};But even if that happens, it's not that bad to have one more wrapped module. |
mischnic
approved these changes
Oct 7, 2023
|
Theoretically we could also only wrap if the actual namespace is imported (eg import *) and not when only named imports are used. Not sure if it makes a big difference. |
devongovett
reviewed
Oct 10, 2023
| } else if !self.in_class { | ||
| if let MemberProp::Ident(prop) = &node.prop { | ||
| self.this_exprs.insert(id!(prop), node.clone()); | ||
| } |
There was a problem hiding this comment.
Do we only want to handle this in exported functions? If you have a non-exported function that uses this we shouldn't need to wrap. You could track which functions use this instead of each property access, and then later check if any of the exported functions used this. Might be more accurate than just looking for property names?
There was a problem hiding this comment.
If we are tracking functions that use this, there would have to be some sort of struct that maps the Ident for the original exported function to every re-assignment of the function to another variable. Discussed this with @imbrian and we were thinking that the overhead from wrapping every time a this is encountered seems better than having to keep a (possibly very large) struct of every possible alias referring back to a function that calls this.
|
FYI: changes don't seem to significantly affect build time (44.27s for this PR, 43.59s for v2) and is a actually faster build for the Atlassian repo we are working to integrate Parcel into @devongovett @mischnic @imbrian |
mischnic
reviewed
Oct 11, 2023
adelchan07
added a commit
that referenced
this pull request
Oct 18, 2023
* recognize 'this' as a potential keyword linking to an export * Add co-authors. Co-authored-by: Eric Eldredge <lettertwo@gmail.com> Co-authored-by: Brian Tedder <btedder@atlassian.com> * update BailoutReason text, fix test name * rename "thises" to "this_exprs" * remove print statements * cleanup * don't wrap when `this` collides in class * move bailouts outside of tracing * add integration test * cleanup integration test * update test: move to commonjs and change search * update naming * Update packages/core/integration-tests/test/scope-hoisting.js Co-authored-by: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com> * move fixture for test into commonjs folder * update link for THisInExport bailout reason * remove duplicate memberProp check * Revert "remove duplicate memberProp check" This reverts commit a017082. * store Ident, Span instead of entire node * move instead of clone self.this_exprs * Cleanup --------- Co-authored-by: achan3 <achan3@atlassian.com> Co-authored-by: Brian Tedder <btedder@atlassian.com> Co-authored-by: Brian Tedder <6571474+imbrian@users.noreply.github.com> Co-authored-by: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com>
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
↪️ Pull Request
Add logic to disable scope hoisting if the "this" key word is pointing to an export module. Existing behavior only de-optimizes when encountering the "export" key word
Resolves issue #7866
💻 Examples
🚨 Test instructions
cargo test✔️ PR Todo