Extract to parser helpers extended by timse · Pull Request #3846 · webpack/webpack

timse · 2017-01-08T13:19:18Z

What kind of change does this PR introduce?

refactoring

Did you add tests for your changes?

Is covered by existing tests

If relevant, link to documentation update:

N/A

Summary

See #3843

Does this PR introduce a breaking change?

No

Other information

extends/continues #3843

TheLarkInn · 2017-01-08T18:12:03Z

Need to sync up branch again after merging #3843

sokra · 2017-01-09T21:28:32Z

 				});
 				IGNORES.forEach(key => {
-					parser.plugin(key, () => true);
+					parser.plugin(key, ParserHelpers.returnTrue);


Really? This is not an optimization?

I probably went a bit too far here 😨
All those lonely () => true, i thought i give them a new home.
(at least it only creates one function instead of many. but thats the best excuse I can come up with.)
that being said, happy to revert :)

Actually doesn't this yield better performance simply for the case of the same hidden class being reused by the JIT Optimizer? Maybe that is just for the case of property access. I'm on the fence really. Either work. Less dedupe but also less flexible.

it should, but probably so small if even measurable, that it is kind of premature if it doesnt have any other advantages.
except those functions get called a lot, because they would need to be created on each call, not sure if that counts for this code?

If you are extracting this into a helper, it's probably better to describe what it actually does in the context of the parser instead of just "returns true". ParserHelpers.skipTraversal?

would ParserHelpers.skipTraversal be acceptable @sokra ?
would describe a lot better what it would do in this context than a simple "return true", no?

well I guess the only problem with skipTraversal is that it is only true in 3 cases, and in the other cases it just "confirmes" can-rename.
Would it be ok for me to add two helper functions, one approve and one skip so they semantically work? given that a simple () => true can have these meanings make the code hard to read imho

Jessidhia · 2017-01-11T01:53:36Z

 				});
 				IGNORES.forEach(key => {
-					parser.plugin(key, () => true);
+					parser.plugin(key, ParserHelpers.returnTrue);


If you are extracting this into a helper, it's probably better to describe what it actually does in the context of the parser instead of just "returns true". ParserHelpers.skipTraversal?

Jessidhia · 2017-01-11T01:57:35Z

-				parser.plugin("call require.config", remove);
-				parser.plugin("call requirejs.config", remove);
+				parser.plugin("call require.config", ParserHelpers.toConstantDependency(";"));
+				parser.plugin("call requirejs.config", ParserHelpers.toConstantDependency(";"));


(unrelated) this probably should resolve to undefined instead of ;; CallExpression is an expression after all.

Good point. But we could do this in a seprate PR, as this also require tests...

Do you want to open one?

sokra · 2017-01-11T13:50:59Z

/home/travis/build/webpack/webpack/lib/ParserHelpers.js
  62:2  error  Missing semicolon  semi
  66:2  error  Missing semicolon  semi

timse · 2017-01-11T21:25:49Z

Urgs will fix!

TheLarkInn · 2017-01-18T15:36:52Z

Let's sync master and see how this guess against new benchmarks for perf against master.

- rename module and file from ModuleParserHelpers to ParserHelpers - change imports and usages - rename addParsedVariable to addParsedVariableToModule to add module context again

- reduces sideeffects as parser is called outside of helper - allows better reuse

convert all usages of new BasicEvaluatedExpression().setBoolean to ParserHelper.evaluateToBoolean

changes usages of toConstantDependency so far

the initial method does not need really another name

…ate methods - skipTraversal if a "return true" indicates to break walking - approve if "return true" means to approve a "parse request" such as "can-rename"

…d of `undefined` this may lead to potential Syntaxerrors as `const x = requirejs.config();` would be transformed to `const x = ;;`

timse · 2017-01-26T00:38:30Z

ill resolve the conflicts

Conflicts: lib/dependencies/AMDPlugin.js

sokra · 2017-01-26T14:57:01Z

/home/travis/build/webpack/webpack/lib/dependencies/CommonJsRequireDependencyParserPlugin.js
  7:7  error  'ConstDependency' is assigned a value but never used  no-unused-vars

timse · 2017-01-26T20:46:08Z

done

sokra · 2017-01-27T07:32:17Z

Thanks

TheLarkInn requested a review from sokra January 8, 2017 18:08

TheLarkInn added the PR: investigating if direction is fine label Jan 8, 2017

timse force-pushed the extract-to-parser-helpers-extended branch from f423f8d to dbf54e7 Compare January 9, 2017 10:04

sokra requested changes Jan 9, 2017

View reviewed changes

Jessidhia reviewed Jan 11, 2017

View reviewed changes

sokra approved these changes Jan 11, 2017

View reviewed changes

sokra modified the milestone: webpack 2.3 Bugfix release Jan 12, 2017

timse force-pushed the extract-to-parser-helpers-extended branch from 9891c24 to cf7bb83 Compare January 12, 2017 08:39

timse mentioned this pull request Jan 12, 2017

Fix require.config issue #3927

Closed

1 task

timse added 14 commits January 21, 2017 13:38

rename ModuleParserHelpers to ParserHelpers

b07a829

- rename module and file from ModuleParserHelpers to ParserHelpers - change imports and usages - rename addParsedVariable to addParsedVariableToModule to add module context again

introduce setTypeof helper to ParserHelpers

e5c8525

add ParserHelpers.expressionIsUnsupported method

086bd9d

refactor setTypeOf to more atomic helper methods

d43cbbd

- reduces sideeffects as parser is called outside of helper - allows better reuse

introduce ParserHelper.evaluateToBoolean

afac22f

convert all usages of new BasicEvaluatedExpression().setBoolean to ParserHelper.evaluateToBoolean

do not implicitly stringify values for toConstantDependency

b066507

changes usages of toConstantDependency so far

move more ConstDependencies to toConstantDependency

965b726

convert more usages of BasicEvaluatedExpression with ParserHelpers

82eec17

add simple returnTrue helper, less function definitions

58e7a2f

improve anonymous function naming

8e896d7

the initial method does not need really another name

split "ParserHelpers.returnTrue" into two more semnantically appropri…

759f738

…ate methods - skipTraversal if a "return true" indicates to break walking - approve if "return true" means to approve a "parse request" such as "can-rename"

apply split methods to their usages

5240430

add missing semicolons

288c7b0

fix issue where requirejs.config() would be evaluated to ; instea…

f46dac8

…d of `undefined` this may lead to potential Syntaxerrors as `const x = requirejs.config();` would be transformed to `const x = ;;`

timse force-pushed the extract-to-parser-helpers-extended branch from cf7bb83 to f46dac8 Compare January 21, 2017 03:00

TheLarkInn added the PR: direction fine label Jan 21, 2017

TheLarkInn added PR: Merge after CI Passes and removed PR: investigating if direction is fine labels Jan 21, 2017

sokra added PR: merge it and removed PR: Merge after CI Passes labels Jan 25, 2017

Merge branch 'master' into extract-to-parser-helpers-extended

41e689a

Merge branch 'master' into extract-to-parser-helpers-extended

dbe7376

Conflicts: lib/dependencies/AMDPlugin.js

sokra closed this Jan 26, 2017

sokra reopened this Jan 26, 2017

sokra added PR: finish details and removed PR: merge it labels Jan 26, 2017

remove unused require

d6e2abf

sokra merged commit 1ff8d69 into webpack:master Jan 27, 2017

timse deleted the extract-to-parser-helpers-extended branch January 27, 2017 07:40

e-cloud mentioned this pull request Mar 8, 2017

refactor(ES6): upgrade CommonJsPlugin to ES6 #3887

Merged

Uh oh!

Conversation

timse commented Jan 8, 2017

Uh oh!

TheLarkInn commented Jan 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheLarkInn Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sokra commented Jan 11, 2017

Uh oh!

timse commented Jan 11, 2017

Uh oh!

TheLarkInn commented Jan 18, 2017

Uh oh!

timse commented Jan 26, 2017

Uh oh!

sokra commented Jan 26, 2017

Uh oh!

timse commented Jan 26, 2017

Uh oh!

sokra commented Jan 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TheLarkInn Jan 10, 2017 •

edited

Loading