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

Expand Support for Eliding Resources at Build Time #94

Merged
merged 4 commits into from
Nov 28, 2018

Conversation

agubler
Copy link
Member

@agubler agubler commented Nov 27, 2018

Type: feature

The following has been addressed in the PR:

Description:

Extend static has build time processing to support additional scenarios such as import.

Support for template literals

For applications that are not built in legacy mode, template literals are not down emitted therefore need to be supported when detecting the static has pragma

`has('flag')`

Support for require with a variable declaration

When an require contains a variable declaration, it should be elided.

"!has('include-my-require')"
var my_require = require('my-require');

The variable declaration is replaced with an undefined variable:

var my_require = undefined;

Support for import syntax

Applications not built in legacy mode will not down emit the import syntax but should be supported.

"!has('include-my-require')"
import 'my-require';

For imports that returns a declaration (either default or named), replaces import with an undefined var:

"!has('include-my-require')"
import defaultValue, { namedValue } from 'my-require';

Is transformed to:

var defaultValue = undefined, namedValue = undefined;

Has Flag Logic

Inverted the logic used by the static has processor to include the next import then the has check resolves as to true, this is more natural and reads like you would expect.

`has('include-foo')`
import 'foo';

Previously this would have elided the foo import, however logically the statement appears to indicate that if include-foo is true then it should import foo. Meaning that the has pragma should be negated to elide the import, i.e. only for cases where foo does not exist.

`!has('include-foo')`
import 'foo';

Consolidation of feature sets

We only leverage a single feature (currently chrome) set that describes modern browsers, therefore we can normalise this to a single modern feature set.

Related to dojo/framework#183

@codecov
Copy link

codecov bot commented Nov 27, 2018

Codecov Report

Merging #94 into master will increase coverage by 0.75%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
+ Coverage   84.72%   85.47%   +0.75%     
==========================================
  Files          33       33              
  Lines        1211     1260      +49     
  Branches      332      353      +21     
==========================================
+ Hits         1026     1077      +51     
+ Misses        185      183       -2
Impacted Files Coverage Δ
src/static-build-loader/loader.ts 99.35% <100%> (+4.03%) ⬆️
src/static-build-loader/getFeatures.ts 91.3% <0%> (-8.7%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f21fb4...45e5cec. Read the comment docs.

if (hasPragma) {
const [, negate, flag] = hasPragma;
comment = ` ${negate}has('${flag}')`;
if (flag in features) {
elideNextImport = negate ? !features[flag] : features[flag];
elideNextImport = negate ? !!features[flag] : !features[flag];
Copy link
Member Author

Choose a reason for hiding this comment

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

Inverts the logic of the has, so now it will only elide the import when the has expression evaluates to false

@agubler agubler requested review from maier49 and matt-gadd and removed request for maier49 November 27, 2018 09:31
@agubler agubler merged commit f6516c0 into dojo:master Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants