Add static has build optimization #172

kitsonk · 2017-07-03T08:29:33Z

Type: feature

The following has been addressed in the PR:

There is a related issue
All code matches the style guide
Unit or Functional tests are included in the PR

Description:

This PR allows build optimization using the has feature detection library. During the build process, any static features that have been asserted are used to replace call expressions that look something like has('feature'). For example, if feature foo was set to true as a static flag, this code:

import has from `@dojo/has/has`;

if (has('foo')) {
  console.log('has foo');
}
else {
  console.log('does not have foo');
}

Would be transformed to this:

import has from `@dojo/has/has`;

if (true) {
  console.log('has foo');
}
else {
  console.log('does not have foo');
}

Then further during the build process, if the build is being minified, uglify will detect the dead code and rewrite it further to something like:

console.log('has foo');

At the command line, users are abstracted from individual flags, and instead supply a set of feature flags they wish to use. The supported sets are located in src/features/*.json. These are specified by the user by setting the -f or --feature flag on the CLI. Multiple feature sets are combined to produce the largest set where the values do not conflict. For example es6-promise is true on chrome but false on ie11. If chrome and ie11 were specified, the values would conflict and this feature would not be statically bound.

A user supplying multiple platforms would look something like this:

$ dojo build -f chrome safari firefox

Which would produce a build that is statically built with features that are common between Chrome, Safari, Firefox.

At the moment, most of the shim library isn't written to have polyfill code within has() guard blocks, therefore it doesn't get removed from the build when the native feature is statically expressed. We will need to revisit those modules in order to create something that builds optimally.

Todo:

- test cases
- readme/documentation

Resolves #142

kitsonk · 2017-07-04T09:11:00Z

Two thoughts on this...

In the original issue, I suggested -t and --target. I am not sure which makes overall better sense. Or even something else?
There is no support for user defined flags. I think that is fairly critical, but I suggest we tackle that as a separate issue/PR after we land this one. I think using .dojorc to specify custom features which will be mixed in to any other targets makes the most sense.

dylans

minor documentation nits/improvements

dylans · 2017-07-04T13:55:50Z

README.md

@@ -79,6 +80,33 @@ If the source file does not follow the pattern `create[custom element]Element`,
 dojo build webpack --element=src/path/to/element.ts --elementPrefix=the-special
 ```

+### Feature optimization
+
+This command supports the ability to optimize code based on statically asserted features.  The tool can search the source code for modules that attempt to detect features using a [`@dojo/has`](https://github.com/dojo/has) type of API.  By supplying a feature set (or sets) on the command line, the build will be optimized to optimize code branches, making the code smaller and slightly more efficient.  This allows targeting of particular platforms.  The tool will not optimize code, when specifying multiple feature sets, they do not align.  It will instead continue to leave that feature to be detected at run-time.


the build will be optimized to optimize code branches, making the code smaller and slightly more efficient. -> the build will optimize code branches, making the code smaller and more efficient.

dylans · 2017-07-04T13:57:24Z

README.md

@@ -79,6 +80,33 @@ If the source file does not follow the pattern `create[custom element]Element`,
 dojo build webpack --element=src/path/to/element.ts --elementPrefix=the-special
 ```

+### Feature optimization
+
+This command supports the ability to optimize code based on statically asserted features.  The tool can search the source code for modules that attempt to detect features using a [`@dojo/has`](https://github.com/dojo/has) type of API.  By supplying a feature set (or sets) on the command line, the build will be optimized to optimize code branches, making the code smaller and slightly more efficient.  This allows targeting of particular platforms.  The tool will not optimize code, when specifying multiple feature sets, they do not align.  It will instead continue to leave that feature to be detected at run-time.


Move this to a separate paragraph and change "The tool will not optimize code, when specifying multiple feature sets, they do not align. It will instead continue to leave that feature to be detected at run-time." to "When specifying multiple feature sets, if they do not align, the tool will not optimize the source code for these feature sets and will instead continue to leave that feature to be detected at run-time."

dylans · 2017-07-04T13:58:49Z

README.md

+|`node`|This feature set represents Node.js 7+[<sup>2</sup>](#note-2)|
+|`safari`|This feature set represents Safari 10+[<sup>2</sup>](#note-2)|
+
+<span id="note-1">[1]:</span> Many of these features were present in earlier versions, but the specific version was the GA release at the time of writing when this was validated.


Maybe also say something to the effect that this isn't specific to Chrome 59+ or Opera 46+, and also supports browsers built on the same codebase such as Brave, Vivaldi, and perhaps Electron?

dylans · 2017-07-04T13:59:42Z

README.md

+
+From the command line, the feature sets are provided to the `-f` or `--feature` argument.  The available feature sets are aligned to platforms.  The currently available feature sets are:
+
+|Feature Set|Description|


People may not be impressed to learn that we have what they might think of a browser sniff testing rather than feature testing, so perhaps it's worth explaining a bit more how these feature test lists were created?

dylans · 2017-07-04T14:00:25Z

src/getFeatures.ts

+		return {};
+	}
+
+	// Reduce the array of loaded features to the largest set of features where the values don't


don't -> do not

dylans · 2017-07-04T14:00:47Z

src/plugins/HasPlugin.ts

+		const features = this._features;
+		const dynamicFlags = new Set<string>();
+
+		// setup the dependencies for the subtitution


subtitution -> substitution

dylans · 2017-07-04T14:01:09Z

src/plugins/HasPlugin.ts

+		compiler.plugin('compilation', (compilation, data) => {
+			// and we want to hook the parser
+			data.normalModuleFactory.plugin('parser', (parser) => {
+				// we need direct access to the AST to properly figure out the has subtitution


subtitution -> substitution

dylans · 2017-07-04T14:01:56Z

src/plugins/util/walk.ts

+/**
+ * Walk a estree AST tree
+ * @param node The root estree Node to start walking
+ * @param param1 An object with an `enter` and `leave` properties


with an -> with

kitsonk · 2017-07-07T14:06:41Z

This now requires dojo/shim#101 for the sets of features to actually be effective.

rorticus · 2017-07-21T14:32:33Z

src/plugins/util/walk.ts

+	});
+
+	if (leave) {
+		leave(node, parent, prop, index);


Probably not important, since it doesn't use shouldSkip, but it feels like leave should be called with the same context as enter? Just for consistency maybe.

Yeah, but we don't want people calling this.skip() thinking that it would do something. So instead I have changed it to null and annotated the this in the types so that no one gets any bright ideas.

Comments addressed.

kitsonk requested review from mwistrand, agubler and rorticus July 4, 2017 09:08

kitsonk changed the title ~~WIP: Add static has build optimization~~ Add static has build optimization Jul 4, 2017

dylans previously requested changes Jul 4, 2017

View reviewed changes

dylans added this to the 2017.07 milestone Jul 4, 2017

rorticus approved these changes Jul 21, 2017

View reviewed changes

kitsonk mentioned this pull request Jul 27, 2017

Targeting of Builds dojo/meta#20

Closed

kitsonk force-pushed the feature-has branch from eeccfbb to 30e1721 Compare August 1, 2017 13:00

dylans modified the milestones: 2017.07, 2017.08 Aug 4, 2017

kitsonk added 10 commits August 14, 2017 08:58

Add static has build optimization

dcbc8f7

Cleanup to allow test to pass

9e6a5ea

More cleanup for tests

0eee6cb

Add to README for build feature optimization

1625a47

Add tests

f12784f

Update flags in line with dojo/shim#101

c434311

Update based on feedback

3d2d29a

Fix expected for getFeatures test

3d01d2b

Breakout HasPlugin to @dojo/static-optimize-plugin

9da4a4d

Cleanup from rebasing

53d03ae

kitsonk force-pushed the feature-has branch from cda5682 to 53d03ae Compare August 14, 2017 08:13

kitsonk added 2 commits August 30, 2017 12:33

Put index in proper location in interface

2316538

Merge remote-tracking branch 'upstream/master' into feature-has

94f1a56

rorticus approved these changes Aug 30, 2017

View reviewed changes

kitsonk merged commit 69b6213 into dojo:master Aug 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add static has build optimization #172

Add static has build optimization #172

kitsonk commented Jul 3, 2017 •

edited

Loading

kitsonk commented Jul 4, 2017

dylans left a comment

dylans Jul 4, 2017

dylans Jul 4, 2017

dylans Jul 4, 2017

dylans Jul 4, 2017

dylans Jul 4, 2017

dylans Jul 4, 2017

dylans Jul 4, 2017

dylans Jul 4, 2017

kitsonk commented Jul 7, 2017

rorticus Jul 21, 2017

kitsonk Aug 1, 2017


		From the command line, the feature sets are provided to the `-f` or `--feature` argument. The available feature sets are aligned to platforms. The currently available feature sets are:

		\|Feature Set\|Description\|

Add static has build optimization #172

Add static has build optimization #172

Conversation

kitsonk commented Jul 3, 2017 • edited Loading

kitsonk commented Jul 4, 2017

dylans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsonk commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsonk commented Jul 3, 2017 •

edited

Loading