Issue 6867

[Janpot]

What kind of change does this PR introduce?

Bug fix for #6867

Did you add tests for your changes?

Yes, always

If relevant, link to documentation update:

Summary

Explain the motivation for making this change. What existing problem does the pull request solve?

Basically, I tried to compile a project with webpack 4, it didn't work.

#6867 contains more info on what's going on exactly.

Does this PR introduce a breaking change?

Only if you rely on it throwing errors when it shouldn't :)

Other information

Don't merge yet, I just realized

export default (class A {

});

suffers from the same problem, and some other code probably too.

[jsf-clabot]

All committers have signed the CLA.

[Jessidhia]

It's possible that this was an attempt to workaround an old acorn bug where it'd incorrectly parse export default function () {} as a function expression.

[ooflorent]

In acorn 4, export default function() { was parsed as a FunctionExpression.
Since acorn 5 it is a FunctionDeclaration with id=null.

[Jessidhia]

I'd feel better if there were tests for each permutation.

// function _declaration_, id is null
// in native implementations the function is named "default"
export default function() {}

// function _expression_, id is null
// in native implementations the function is named "default"
export default (function() {})

// function _declaration_, id is "foo"
export default function foo() {
  // this not working at runtime is currently a bug in webpack 😝
  foo = function() {}
})

// function _expression_, id is "bar"
export default (function bar() {})

[Jessidhia]

These same permutations also apply to class declarations, but we probably don't need to test them here 🤔

[ooflorent]

Acorn did the same for ClassDeclaration and ClassExpression. It may be worth testing it.

[sokra]

see @Kovensky's review

[Janpot]

@Kovensky @sokra

export default (class A {

});

is breaking too, how do you feel about

		if (
			statement.declaration.id &&
			!statement.declaration.type.endsWith("Expression")
		)

? is that too hacky?

[ooflorent]

@Janpot yes it is. You could test both node types.

[webpack-bot]

@Janpot Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

[Jessidhia]

Nah, it's definitely a separate issue. Webpack doesn't actually detect when an export is mutable (mutated) or immutable, it just guesses that function and const are immutable and everything else is mutable.

[Janpot]

Could it be that the coverage went down, just because the amount of lines increased more than the amount of statements?

[webpack-bot]

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

[sokra]

Could you move this test case from configCases to cases?

[Janpot]

done

[Jessidhia]

Other than the test messages looks OK

[Jessidhia]

class declaration / class expression; same for lines 30, 38 and 45

[sokra]

Thanks