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

Jump to bottom

Support array in package.json's sideEffects property #1766

Merged
merged 1 commit into from Jul 21, 2018
Merged

Support array in package.json's sideEffects property #1766

merged 1 commit into from Jul 21, 2018

Conversation

ahocevar
Copy link
Contributor

This pull request implements array support for the package.json's sideEffects property, like webpack does. Example:

"sideEffects": ["./a.js"]

Fixes #1566.

@ahocevar ahocevar mentioned this pull request Jul 20, 2018
Closed
fathyb
fathyb reviewed Jul 21, 2018
View reviewed changes
src/Asset.js
@@ -125,7 +125,7 @@ class Asset {

async getPackage() {
if (!this._package) {
this._package = await this.getConfig(['package.json']);
this._package = await this.resolver.findPackage(path.dirname(this.name));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed?

Copy link
Contributor Author

@ahocevar ahocevar Jul 21, 2018 •
edited

Choose a reason for hiding this comment

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

Because findPackage() will add a pkgdir property to the package, which is needed later for globbing the sideEffects array entries.

src/packagers/JSConcatPackager.js Outdated
case 'boolean':
return sideEffects;
case 'string':
if (process.platform === 'win32') {
Copy link
Contributor

@fathyb fathyb Jul 21, 2018 •
edited

Choose a reason for hiding this comment

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

You can use path.normalize(sideEffects) instead of manually checking for windows and using replace
Oh, you're doing the opposite, nevermind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking again, this is not necessary at all. "sideEffects": ["foo\\bar.js"] is not supported in package.json anyway.

src/packagers/JSConcatPackager.js Outdated
sideEffects = sideEffects.replace(/\\/g, '/');
}
return mm.isMatch(
path.relative((await asset.getPackage()).pkgdir, asset.name),
Copy link
Contributor

Choose a reason for hiding this comment

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

You can save some I/O and async overhead by setting pkgdir in cacheData preemptively in hoist.js like we do for sideEffects :

parcel/src/scope-hoisting/hoist.js

Lines 34 to 38 in 7423541

asset.cacheData.imports = asset.cacheData.imports || Object.create(null);
asset.cacheData.exports = asset.cacheData.exports || Object.create(null);
asset.cacheData.wildcards = asset.cacheData.wildcards || [];
asset.cacheData.sideEffects =
asset._package && asset._package.sideEffects;

src/packagers/JSConcatPackager.js Outdated
@@ -23,6 +24,31 @@ const helpers =
.readFileSync(path.join(__dirname, '../builtins/helpers.js'), 'utf8')
.trim() + '\n';

async function hasSideEffects(asset, sideEffects) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: defaulting sideEffects using {sideEffects} = asset.cacheData could make the if conditions using it cleaner (await hasSideEffects(asset))

@ahocevar ahocevar force-pushed the fine-grained-sideeffects branch 2 times, most recently from d7ccd48 to 69859fb Compare July 21, 2018 21:28
@ahocevar
Copy link
Contributor Author

@fathyb Thanks for the review. I have addressed all your comments.

devongovett
devongovett reviewed Jul 21, 2018
View reviewed changes
src/packagers/JSConcatPackager.js Outdated
@@ -23,6 +24,25 @@ const helpers =
.readFileSync(path.join(__dirname, '../builtins/helpers.js'), 'utf8')
.trim() + '\n';

function hasSideEffects(asset, {sideEffects} = asset.cacheData) {
Copy link
Member

Choose a reason for hiding this comment

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

this function could probably go in hoist.js. That way asset.cacheData.sideEffects could always be a boolean computed using this function. Might be slightly faster, especially since we can cache the results.

devongovett
devongovett reviewed Jul 21, 2018
View reviewed changes
src/Asset.js Outdated
this._package.pkgdir = path.dirname(
await config.resolve(this.name, ['package.json'])
);
}
Copy link
Member

Choose a reason for hiding this comment

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

I actually liked your original solution using the resolver. This one will result in two lookups.

@ahocevar
Copy link
Contributor Author

Thanks @devongovett for your review. I have addressed your comments as well now.

fathyb
fathyb approved these changes Jul 21, 2018
View reviewed changes
Copy link
Contributor

@fathyb fathyb left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for working on this 👍

@devongovett devongovett merged commit 26dd8c8 into parcel-bundler:master Jul 21, 2018
@ahocevar ahocevar deleted the fine-grained-sideeffects branch July 21, 2018 22:10
devongovett pushed a commit that referenced this pull request Oct 15, 2018
@ahocevar @devongovett
Support array in package.json's sideEffects property (#1766)
7514852
devongovett pushed a commit that referenced this pull request Oct 15, 2018
@ahocevar @devongovett
Support array in package.json's sideEffects property (#1766)
a847171
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devongovett devongovett devongovett approved these changes

@fathyb fathyb fathyb approved these changes

Assignees
No one assigned
Labels
🙋‍♀️ Feature 🌳 Scope Hoisting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ahocevar @devongovett @fathyb