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

Add support for more complex selectors for sideEffects #6074

Closed
TheLarkInn opened this Issue Dec 5, 2017 · 19 comments

Comments

Projects
None yet
7 participants
@TheLarkInn
Member

TheLarkInn commented Dec 5, 2017

Do you want to request a feature or report a bug?
Feature

What is the current behavior?
webpack searched for "sideEffects": false in package.json to perform the extra optimizations for esm modules. However if false is a string, the optimization isn't detected.

We should probably (in the case that package authors might mistake this as "false" vs false) put the wrong value in, and theres no simple way to detect (as a dev) why the 'deopt' occurs.
If the current behavior is a bug, please provide the steps to reproduce.

What is the expected behavior?
"sideEffects": "false" in package.json works the same as "sideEffects": false

If this is a feature request, what is motivation or use case for changing the behavior?
Motivation described above.

Please mention other relevant information such as the browser version, Node.js version, webpack version and Operating System.
webpack 4.0.0-alpha

@TheLarkInn TheLarkInn added this to the webpack 4 milestone Dec 5, 2017

CWelshE pushed a commit to CWelshE/webpack that referenced this issue Dec 6, 2017

Cody Welsh
fix(SideEffectsFlag): Allow "false" and `false`
webpack#6074

Slightly modifies `SideEffectsFlagPlugin` to consider the string
`"false"` to be a valid input, whereas it previously only allowed for
the boolean `false`.
@CWelshE

This comment has been minimized.

CWelshE commented Dec 6, 2017

I'm not quite sure how to architect the test case(s) for this issue. However, I am looking into existing tests to see if I can reuse anything.

@sokra

This comment has been minimized.

Member

sokra commented Dec 6, 2017

"sideEffects": "false" should mark every module as side effect free except a file named false

@CWelshE

This comment has been minimized.

CWelshE commented Dec 6, 2017

Oh, I may have completely misunderstood the issue, then. Am I just to implement the case where specifying a String instead of a Boolean value tells webpack which files are known to have effects? If so, should we allow Arrays of strings as well?

@sokra

This comment has been minimized.

Member

sokra commented Dec 7, 2017

yep Array of string should be allowed too.

It should support glob expressions too.

"sideEffects": [
  "src/**/*.js"
  "./lib/abc.js"
]

@sokra sokra changed the title from Detect sideEffects: "false" coerced bool from string to Add support for complexer selectors for sideEffects Dec 7, 2017

@TheLarkInn TheLarkInn changed the title from Add support for complexer selectors for sideEffects to Add support for more complex selectors for sideEffects Dec 7, 2017

@CWelshE

This comment has been minimized.

CWelshE commented Dec 7, 2017

Got it - I'll work on that, and inquire further if I have any difficulties.

@CWelshE

This comment has been minimized.

CWelshE commented Dec 11, 2017

I've been trying to decipher some segments of this particular Plugin instance, but I might be lacking some context. The first ambiguous thing in here (to me) is the usage of harmony, so I ran off to check out examples. As far as I can tell, this is part of how webpack keeps collisions out of imports. I can tell that it will take some time before I grasp the larger web of interaction, so I'll proceed with that assumption (unless somebody corrects me, which would be welcome anyways).

In any case, do I have access to the name of the module or the filename when normal-module-factory returns the data object? I'm assuming that's how I will be comparing it to the array of strings the user provides. It looks like I may be able to call __filename from the Node API, but it's unclear to me whether that would report what I want inside of this context.

@sokra

This comment has been minimized.

Member

sokra commented Dec 11, 2017

https://github.com/webpack/webpack/blob/next/lib/optimize/SideEffectsFlagPlugin.js#L19

resolveData.relativePath is the relative path of the file relative to the package.json.

@CWelshE

This comment has been minimized.

CWelshE commented Dec 13, 2017

Oh, thanks for that. This will make things much easier.

@CWelshE

This comment has been minimized.

CWelshE commented Dec 18, 2017

Apologies for the gap!

Here's where I am at:

  • glob expansion via glob (run against the user strings if they both contain * and there was no prior match for the current module);
  • matches for relative paths.

I need to make tests for:

  • arbitrary relative file paths
  • glob paths of arbitrary depths and file extensions

I guess there might be some potential slowdown if there's a significantly larger glob than normal. I'll measure performance if it's wanted. Otherwise, I'll continue immersing myself to learn webpack contribution knowledge. 😃

@sokra

This comment has been minimized.

Member

sokra commented Dec 19, 2017

Instead of using glob: Convert the glob expression into a RegExp and just test the relative path with it.

src/**/abc/*.js => /\/src\/.+\/abc\/[^\/]+\.js($|\/)/

./src/def => /^\./src\/def($|\/)/

@CWelshE

This comment has been minimized.

CWelshE commented Dec 21, 2017

Oh, interesting. I'd not thought of that. I'll convert it over accordingly!

Quick question before I go too far: How complete should the conversion be? Are we only looking for **, *.extension and *? Looks like you also provided a case for just a whole directory, so I assume that's one too. (Thanks for the help!)

@sokra

This comment has been minimized.

Member

sokra commented Dec 22, 2017

@abiduzz420

This comment has been minimized.

abiduzz420 commented Jan 25, 2018

Hi @CWelshE I want to know if you are still working on it ?

@reergymerej

This comment has been minimized.

Member

reergymerej commented Jan 25, 2018

@abiduzz420 I've got a PR that needs a modification to the glob feature. It's a pretty small change, but I'm not able to follow up on it right now.

@TheLarkInn

This comment has been minimized.

Member

TheLarkInn commented Feb 6, 2018

@reergymerej is there any notes you could provide to assist others? Happy to help find another contributor.

@reergymerej

This comment has been minimized.

Member

reergymerej commented Feb 6, 2018

The PR is #6334. I just needed to switch the library doing the glob checking to minimatch and update tests. The test structure is already there. It's should be as easy as changing the library in the source and the assertions to go along with those in the PR review notes.

@jevancc

This comment has been minimized.

Member

jevancc commented Feb 18, 2018

Hi, may I take up this issue and continue what @reergymerej has done?

@reergymerej

This comment has been minimized.

Member

reergymerej commented Feb 19, 2018

@jevan0307, please do. I pushed the work in progress I had. 2b45e65

@massicattaneo

This comment has been minimized.

massicattaneo commented Oct 24, 2018

I'm trying to use the tree shaking on 2 different bundles that imports different fn from the same code.
imaging a "shared.js" like this:

export function square(x) {
    return x * x;
}

export function cube(x) {
    return x * x * x;
}

And 2 bundles 1 importing {square} and the other importing {cube} I was expecting to see the tree shake working but it happen to me that the both bundles contains the whole "share.js" exports. Am I doing something wrong?

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