Skip to content

Commit d154d97

Browse files
author
Robert Jackson
committed
Remove error throwing for svelte guarded features.
Adding the error is error prone due to the ability to include arbitrary JS in conditionals, and provides a false sense of security due to the fact that a deprecated feature flag could be used in many locations other than a conditional. This changes the strategy for svelte features to: * **always** replace the imported deprecated flag with its boolean value * **never** add an arbitrary error * **always** remove feature source import statements that have no specifiers (previously only done in prod builds)
1 parent 65ae930 commit d154d97

File tree

7 files changed

+95
-66
lines changed

7 files changed

+95
-66
lines changed

fixtures/development-svelte-builds/expectation6.js

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,28 @@
1-
import { DEPRECATED_PARTIALS, DEPRECATED_CONTROLLERS } from '@ember/features';
21

3-
export let PartialComponentManager;
4-
if (DEPRECATED_PARTIALS) {
5-
throw new Error('You indicated you don\'t have any deprecations, however you are relying on DEPRECATED_PARTIALS.');
62

3+
export let PartialComponentManager;
4+
if (false /* DEPRECATED_PARTIALS */) {
75
PartialComponentManager = class {
86
constructor() {
97
this.isDone = true;
108
}
119
};
1210
}
1311

14-
if (DEPRECATED_PARTIALS && someOtherThing()) {
15-
throw new Error('You indicated you don\'t have any deprecations, however you are relying on DEPRECATED_PARTIALS.');
16-
12+
if (false /* DEPRECATED_PARTIALS */ && someOtherThing()) {
1713
doStuff();
1814
}
1915

16+
if (!false /* DEPRECATED_PARTIALS */ && someOtherThing()) {
17+
doStuff2();
18+
}
19+
20+
if (false /* DEPRECATED_PARTIALS */ === false && someOtherThing()) {
21+
doStuff3();
22+
}
23+
2024
export let ObjectController;
21-
if (DEPRECATED_CONTROLLERS) {
25+
if (true /* DEPRECATED_CONTROLLERS */) {
2226
ObjectController = class {
2327
constructor() {
2428
this.isDoneAsWell = true;

fixtures/development-svelte-builds/expectation7.js

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
1-
import { DEPRECATED_PARTIALS, DEPRECATED_CONTROLLERS } from '@ember/features';
21
export let PartialComponentManager;
32

4-
if (DEPRECATED_PARTIALS) {
5-
throw new Error("You indicated you don't have any deprecations, however you are relying on DEPRECATED_PARTIALS.");
3+
if (false
4+
/* DEPRECATED_PARTIALS */
5+
) {
66
PartialComponentManager = class {
77
constructor() {
88
this.isDone = true;
@@ -11,14 +11,29 @@ if (DEPRECATED_PARTIALS) {
1111
};
1212
}
1313

14-
if (DEPRECATED_PARTIALS && someOtherThing()) {
15-
throw new Error("You indicated you don't have any deprecations, however you are relying on DEPRECATED_PARTIALS.");
14+
if (false
15+
/* DEPRECATED_PARTIALS */
16+
&& someOtherThing()) {
1617
doStuff();
1718
}
1819

20+
if (!false
21+
/* DEPRECATED_PARTIALS */
22+
&& someOtherThing()) {
23+
doStuff2();
24+
}
25+
26+
if (false
27+
/* DEPRECATED_PARTIALS */
28+
=== false && someOtherThing()) {
29+
doStuff3();
30+
}
31+
1932
export let ObjectController;
2033

21-
if (DEPRECATED_CONTROLLERS) {
34+
if (true
35+
/* DEPRECATED_CONTROLLERS */
36+
) {
2237
ObjectController = class {
2338
constructor() {
2439
this.isDoneAsWell = true;

fixtures/development-svelte-builds/sample.js

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,14 @@ if (DEPRECATED_PARTIALS && someOtherThing()) {
1313
doStuff();
1414
}
1515

16+
if (!DEPRECATED_PARTIALS && someOtherThing()) {
17+
doStuff2();
18+
}
19+
20+
if (DEPRECATED_PARTIALS === false && someOtherThing()) {
21+
doStuff3();
22+
}
23+
1624
export let ObjectController;
1725
if (DEPRECATED_CONTROLLERS) {
1826
ObjectController = class {

fixtures/production-svelte-builds/expectation6.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
11

22

33
export let PartialComponentManager;
4-
if (false) {
4+
if (false /* DEPRECATED_PARTIALS */) {
55
PartialComponentManager = class {
66
constructor() {
77
this.isDone = true;
88
}
99
};
1010
}
1111

12-
if (false && someOtherThing()) {
12+
if (false /* DEPRECATED_PARTIALS */ && someOtherThing()) {
1313
doStuff();
1414
}
1515

1616
export let ObjectController;
17-
if (true) {
17+
if (true /* DEPRECATED_CONTROLLERS */) {
1818
ObjectController = class {
1919
constructor() {
2020
this.isDoneAsWell = true;

fixtures/production-svelte-builds/expectation7.js

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
export let PartialComponentManager;
22

3-
if (false) {
3+
if (false
4+
/* DEPRECATED_PARTIALS */
5+
) {
46
PartialComponentManager = class {
57
constructor() {
68
this.isDone = true;
@@ -9,13 +11,17 @@ if (false) {
911
};
1012
}
1113

12-
if (false && someOtherThing()) {
14+
if (false
15+
/* DEPRECATED_PARTIALS */
16+
&& someOtherThing()) {
1317
doStuff();
1418
}
1519

1620
export let ObjectController;
1721

18-
if (true) {
22+
if (true
23+
/* DEPRECATED_CONTROLLERS */
24+
) {
1925
ObjectController = class {
2026
constructor() {
2127
this.isDoneAsWell = true;

src/index.js

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,6 @@ const Macros = require('./utils/macros');
55
const normalizeOptions = require('./utils/normalize-options').normalizeOptions;
66

77
function macros(babel) {
8-
const t = babel.types;
9-
108
let options;
119

1210
return {
@@ -15,7 +13,7 @@ function macros(babel) {
1513
Program: {
1614
enter(path, state) {
1715
options = normalizeOptions(state.opts);
18-
this.macroBuilder = new Macros(t, options);
16+
this.macroBuilder = new Macros(babel, options);
1917

2018
let body = path.get('body');
2119

src/utils/macros.js

Lines changed: 41 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@ const DEBUG = 'DEBUG';
66
const SUPPORTED_MACROS = ['assert', 'deprecate', 'warn', 'log'];
77

88
module.exports = class Macros {
9-
constructor(t, options) {
9+
constructor(babel, options) {
10+
this.babel = babel;
1011
this.localDebugBindings = [];
1112
this.envFlagBindings = [];
1213
this.hasEnvFlags = false;
@@ -19,7 +20,7 @@ module.exports = class Macros {
1920
this.svelteVersions = options.svelte;
2021
this.featureFlags = options.features || [];
2122
this.debugHelpers = options.externalizeHelpers || {};
22-
this.builder = new Builder(t, {
23+
this.builder = new Builder(babel.types, {
2324
module: this.debugHelpers.module,
2425
global: this.debugHelpers.global,
2526
assertPredicateIndex: options.debugTools.assertPredicateIndex,
@@ -88,43 +89,42 @@ module.exports = class Macros {
8889

8990
_inlineSvelteFlags(path) {
9091
let svelteMap = this.svelteMap;
91-
let envFlags = this.envFlags;
92-
let builder = this.builder;
92+
let t = this.babel.types;
93+
94+
function buildIdentifier(value, name) {
95+
let replacement = t.booleanLiteral(value);
96+
97+
// when we only support babel@7 we should change this
98+
// to `path.addComment` or `t.addComment`
99+
let comment = {
100+
type: 'CommentBlock',
101+
value: ` ${name} `,
102+
leading: false,
103+
trailing: true,
104+
};
105+
replacement.trailingComments = [comment];
106+
107+
return replacement;
108+
}
93109

94110
let sources = Object.keys(svelteMap);
95111
sources.forEach(source => {
96-
Object.keys(svelteMap[source]).forEach(flag => {
112+
let flagsForSource = svelteMap[source];
113+
114+
for (let flag in flagsForSource) {
115+
let flagValue = flagsForSource[flag];
116+
97117
let binding = path.scope.getBinding(flag);
98118
if (binding !== undefined) {
99119
binding.referencePaths.forEach(p => {
100-
let t = builder.t;
101-
// in debug builds add an error after a conditional (to ensure if the
102-
// specific branch is taken, an error is thrown)
103-
if (envFlags.DEBUG && svelteMap[source][flag] === false) {
104-
let parentIfStatement = p.find(p => p.isIfStatement());
105-
if (parentIfStatement) {
106-
let consequent = parentIfStatement.get('consequent');
107-
consequent.unshiftContainer(
108-
'body',
109-
t.throwStatement(
110-
t.newExpression(t.identifier('Error'), [
111-
t.stringLiteral(
112-
`You indicated you don't have any deprecations, however you are relying on ${flag}.`
113-
),
114-
])
115-
)
116-
);
117-
}
118-
} else if (envFlags.DEBUG === false) {
119-
p.replaceWith(t.booleanLiteral(svelteMap[source][flag]));
120-
}
120+
let replacement = buildIdentifier(flagValue, flag);
121+
122+
p.replaceWith(replacement);
121123
});
122124

123-
if (!envFlags.DEBUG && binding) {
124-
binding.path.remove();
125-
}
125+
binding.path.remove();
126126
}
127-
});
127+
}
128128
});
129129
}
130130

@@ -183,19 +183,17 @@ module.exports = class Macros {
183183
_cleanImports(path) {
184184
let body = path.get('body');
185185

186-
if (!this.envFlags.DEBUG) {
187-
for (let i = 0; i < body.length; i++) {
188-
let decl = body[i];
189-
190-
if (this.builder.t.isImportDeclaration(decl)) {
191-
let source = decl.node.source.value;
192-
if (this.featureSources.indexOf(source) > -1) {
193-
if (decl.node.specifiers.length > 0) {
194-
this._detectForeignFeatureFlag(decl.node.specifiers, source);
195-
} else {
196-
decl.remove();
197-
break;
198-
}
186+
for (let i = 0; i < body.length; i++) {
187+
let decl = body[i];
188+
189+
if (this.builder.t.isImportDeclaration(decl)) {
190+
let source = decl.node.source.value;
191+
if (this.featureSources.indexOf(source) > -1) {
192+
if (decl.node.specifiers.length > 0) {
193+
this._detectForeignFeatureFlag(decl.node.specifiers, source);
194+
} else {
195+
decl.remove();
196+
break;
199197
}
200198
}
201199
}

0 commit comments

Comments
 (0)