From 218c2619b86d59d0c5f93c369437848612216825 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Fri, 7 Feb 2020 16:21:59 -0800 Subject: [PATCH] [BUGFIX lts] Workaround for integer literals Workaround for the Glimmer VM bug which encodes/decodes integer literals correctly. See #15635. Based on #18484 and https://github.com/emberjs/ember.js/pull/18484#issuecomment-566276195 Fixes #15635. Closes #18484. Co-authored-by: Saravana Kumar V --- .../-internals/glimmer/lib/helpers/-i.ts | 12 ++ .../@ember/-internals/glimmer/lib/resolver.ts | 2 + .../glimmer/tests/integration/content-test.js | 186 ++++++++++++------ .../lib/plugins/index.ts | 2 + .../lib/plugins/safe-integers-bugfix.ts | 87 ++++++++ 5 files changed, 230 insertions(+), 59 deletions(-) create mode 100644 packages/@ember/-internals/glimmer/lib/helpers/-i.ts create mode 100644 packages/ember-template-compiler/lib/plugins/safe-integers-bugfix.ts diff --git a/packages/@ember/-internals/glimmer/lib/helpers/-i.ts b/packages/@ember/-internals/glimmer/lib/helpers/-i.ts new file mode 100644 index 00000000000..a0a10ecb17f --- /dev/null +++ b/packages/@ember/-internals/glimmer/lib/helpers/-i.ts @@ -0,0 +1,12 @@ +import { assert } from '@ember/debug'; +import { CapturedArguments, VM, VMArguments } from '@glimmer/interfaces'; +import { HelperRootReference } from '@glimmer/reference'; + +function i({ positional }: CapturedArguments): number { + assert('[BUG] -i takes a single string', typeof positional.at(0).value() === 'string'); + return parseInt(positional.at(0).value() as string, 10); +} + +export default function(args: VMArguments, vm: VM) { + return new HelperRootReference(i, args.capture(), vm.env); +} diff --git a/packages/@ember/-internals/glimmer/lib/resolver.ts b/packages/@ember/-internals/glimmer/lib/resolver.ts index 196a75e7245..2d6a37cd6c6 100644 --- a/packages/@ember/-internals/glimmer/lib/resolver.ts +++ b/packages/@ember/-internals/glimmer/lib/resolver.ts @@ -23,6 +23,7 @@ import InternalComponentManager, { import { TemplateOnlyComponentDefinition } from './component-managers/template-only'; import { isHelperFactory, isSimpleHelper } from './helper'; import { default as componentAssertionHelper } from './helpers/-assert-implicit-component-helper-argument'; +import { default as parseIntHelper } from './helpers/-i'; import { default as inputTypeHelper } from './helpers/-input-type'; import { default as normalizeClassHelper } from './helpers/-normalize-class'; import { default as trackArray } from './helpers/-track-array'; @@ -250,6 +251,7 @@ const BUILTINS_HELPERS: IBuiltInHelpers = { unless: inlineUnless, '-hash': hash, '-each-in': eachIn, + '-i': parseIntHelper, '-input-type': inputTypeHelper, '-normalize-class': normalizeClassHelper, '-track-array': trackArray, diff --git a/packages/@ember/-internals/glimmer/tests/integration/content-test.js b/packages/@ember/-internals/glimmer/tests/integration/content-test.js index 1d04465daab..e8042919aa6 100644 --- a/packages/@ember/-internals/glimmer/tests/integration/content-test.js +++ b/packages/@ember/-internals/glimmer/tests/integration/content-test.js @@ -10,61 +10,141 @@ import { HAS_NATIVE_SYMBOL } from '@ember/-internals/utils'; import { constructStyleDeprecationMessage } from '@ember/-internals/views'; import { Component, SafeString, htmlSafe } from '../utils/helpers'; -moduleFor( - 'Static content tests', - class extends RenderingTestCase { - ['@test it can render a static text node']() { - this.render('hello'); - let text1 = this.assertTextNode(this.firstChild, 'hello'); +const EMPTY = Object.freeze({}); - runTask(() => this.rerender()); +const LITERALS = [ + ['foo', 'foo', '"foo"'], + [undefined, EMPTY], + [null, EMPTY], + [true, 'true'], + [false, 'false'], + [0, '0'], + [-0, '0', '-0'], + [1, '1'], + [-1, '-1'], + [0.0, '0', '0.0'], + [0.5, '0.5', '0.5'], + [0.5, '0.5', '0.500000000000000000000000000000'], - let text2 = this.assertTextNode(this.firstChild, 'hello'); + // Kris Selden: that is a good one because it is above that 3 bit area, + // but the shifted < 0 check doesn't return true: + // https://github.com/glimmerjs/glimmer-vm/blob/761e78b2bef5de8b9b19ae5fb296380c21959ef8/packages/%40glimmer/opcode-compiler/lib/opcode-builder/encoder.ts#L277 + [536870912, '536870912', '536870912'], +]; - this.assertSameNode(text1, text2); - } +let i = Number.MAX_SAFE_INTEGER; - ['@test it can render a static element']() { - this.render('

hello

'); - let p1 = this.assertElement(this.firstChild, { tagName: 'p' }); - let text1 = this.assertTextNode(this.firstChild.firstChild, 'hello'); +while (i > 1) { + LITERALS.push([i, `${i}`, `${i}`]); + i = Math.round(i / 2); +} - runTask(() => this.rerender()); +i = Number.MIN_SAFE_INTEGER; - let p2 = this.assertElement(this.firstChild, { tagName: 'p' }); - let text2 = this.assertTextNode(this.firstChild.firstChild, 'hello'); +while (i < -1) { + LITERALS.push([i, `${i}`, `${i}`]); + i = Math.round(i / 2); +} - this.assertSameNode(p1, p2); - this.assertSameNode(text1, text2); - } +class StaticContentTest extends RenderingTestCase { + ['@test it can render a static text node']() { + this.render('hello'); + let text1 = this.assertTextNode(this.firstChild, 'hello'); - ['@test it can render a static template']() { - let template = ` -
-

Welcome to Ember.js

-
-
-

Why you should use Ember.js?

-
    -
  1. It's great
  2. -
  3. It's awesome
  4. -
  5. It's Ember.js
  6. -
-
- - `; + runTask(() => this.rerender()); - this.render(template); - this.assertHTML(template); + let text2 = this.assertTextNode(this.firstChild, 'hello'); - runTask(() => this.rerender()); + this.assertSameNode(text1, text2); + } - this.assertHTML(template); - } + ['@test it can render a static element']() { + this.render('

hello

'); + let p1 = this.assertElement(this.firstChild, { tagName: 'p' }); + let text1 = this.assertTextNode(this.firstChild.firstChild, 'hello'); + + runTask(() => this.rerender()); + + let p2 = this.assertElement(this.firstChild, { tagName: 'p' }); + let text2 = this.assertTextNode(this.firstChild.firstChild, 'hello'); + + this.assertSameNode(p1, p2); + this.assertSameNode(text1, text2); } -); + + ['@test it can render a static template']() { + let template = ` +
+

Welcome to Ember.js

+
+
+

Why you should use Ember.js?

+
    +
  1. It's great
  2. +
  3. It's awesome
  4. +
  5. It's Ember.js
  6. +
+
+ + `; + + this.render(template); + this.assertHTML(template); + + runTask(() => this.rerender()); + + this.assertHTML(template); + } +} + +class StaticContentTestGenerator { + constructor(cases, tag = '@test') { + this.cases = cases; + this.tag = tag; + } + + generate([value, expected, label]) { + let tag = this.tag; + label = label || value; + + return { + [`${tag} rendering {{${label}}}`]() { + this.render(`{{${label}}}`); + + if (expected === EMPTY) { + this.assertHTML(''); + } else { + this.assertHTML(expected); + } + + this.assertStableRerender(); + }, + + [`${tag} rendering {{to-js ${label}}}`](assert) { + this.registerHelper('to-js', ([actual]) => { + assert.strictEqual(actual, value); + return actual; + }); + + this.render(`{{to-js ${label}}}`); + + if (expected === EMPTY) { + this.assertHTML(''); + } else { + this.assertHTML(expected); + } + + this.assertStableRerender(); + }, + }; + } +} + +applyMixins(StaticContentTest, new StaticContentTestGenerator(LITERALS)); + +moduleFor('Static content tests', StaticContentTest); class DynamicContentTest extends RenderingTestCase { /* abstract */ @@ -588,9 +668,7 @@ class DynamicContentTest extends RenderingTestCase { } } -const EMPTY = {}; - -class ContentTestGenerator { +class DynamicContentTestGenerator { constructor(cases, tag = '@test') { this.cases = cases; this.tag = tag; @@ -639,18 +717,8 @@ class ContentTestGenerator { } } -const SharedContentTestCases = new ContentTestGenerator([ - ['foo', 'foo'], - [0, '0'], - [-0, '0', '-0'], - [1, '1'], - [-1, '-1'], - [0.0, '0', '0.0'], - [0.5, '0.5'], - [undefined, EMPTY], - [null, EMPTY], - [true, 'true'], - [false, 'false'], +const SharedContentTestCases = new DynamicContentTestGenerator([ + ...LITERALS, [NaN, 'NaN'], [new Date(2000, 0, 1), String(new Date(2000, 0, 1)), 'a Date object'], [Infinity, 'Infinity'], @@ -679,7 +747,7 @@ const SharedContentTestCases = new ContentTestGenerator([ ['MaxJames', 'MaxJames'], ]); -let GlimmerContentTestCases = new ContentTestGenerator([ +let GlimmerContentTestCases = new DynamicContentTestGenerator([ [Object.create(null), EMPTY, 'an object with no toString'], ]); diff --git a/packages/ember-template-compiler/lib/plugins/index.ts b/packages/ember-template-compiler/lib/plugins/index.ts index 07266ab0bdb..35d315c6a03 100644 --- a/packages/ember-template-compiler/lib/plugins/index.ts +++ b/packages/ember-template-compiler/lib/plugins/index.ts @@ -5,6 +5,7 @@ import AssertLocalVariableShadowingHelperInvocation from './assert-local-variabl import AssertReservedNamedArguments from './assert-reserved-named-arguments'; import AssertSplattributeExpressions from './assert-splattribute-expression'; import DeprecateSendAction from './deprecate-send-action'; +import SafeIntegersBugfix from './safe-integers-bugfix'; import TransformActionSyntax from './transform-action-syntax'; import TransformAttrsIntoArgs from './transform-attrs-into-args'; import TransformComponentInvocation from './transform-component-invocation'; @@ -41,6 +42,7 @@ const transforms: Array = [ AssertSplattributeExpressions, TransformEachTrackArray, TransformWrapMountAndOutlet, + SafeIntegersBugfix, ]; if (SEND_ACTION) { diff --git a/packages/ember-template-compiler/lib/plugins/safe-integers-bugfix.ts b/packages/ember-template-compiler/lib/plugins/safe-integers-bugfix.ts new file mode 100644 index 00000000000..5df3b120f9e --- /dev/null +++ b/packages/ember-template-compiler/lib/plugins/safe-integers-bugfix.ts @@ -0,0 +1,87 @@ +import { AST, ASTPlugin, ASTPluginEnvironment } from '@glimmer/syntax'; +import { MustacheStatement, NumberLiteral } from '@glimmer/syntax/dist/types/lib/types/nodes'; + +/** + @module ember +*/ + +/** + A Glimmer2 AST transformation that replaces all instances of + + ```handlebars + {{987654321}} + ``` + + to + + ```handlebars + {{-i "987654321"}} + ``` + + as well as other integer number literals in sexp arguments, etc. + + The version of Glimmer VM we are using has a bug that encodes + certain integers incorrectly. This forces them into strings and + use `{{-i}}` (which is a wrapper around `parseInt`) to decode + them manually as a workaround. + + This should be removed when the Glimmer VM bug is fixed. + + @private + @class SafeIntegersBugfix +*/ + +export default function safeIntegersBugfix(env: ASTPluginEnvironment): ASTPlugin { + let { builders: b } = env.syntax; + + return { + name: 'safe-integers-bugfix', + + visitor: { + MustacheStatement(node: AST.MustacheStatement): AST.MustacheStatement | undefined { + if (!requiresWorkaround(node)) { + return; + } + + return b.mustache( + '-i', + [b.string(String(node.path.value))], + undefined, + !node.escaped, + node.loc + ); + }, + + NumberLiteral(node: AST.NumberLiteral): AST.SubExpression | undefined { + if (!requiresWorkaround(node)) { + return; + } + + return b.sexpr('-i', [b.string(String(node.value))], undefined, node.loc); + }, + }, + }; +} + +type NumberLiteralMustacheStatement = MustacheStatement & { path: NumberLiteral }; + +function requiresWorkaround(node: AST.MustacheStatement): node is NumberLiteralMustacheStatement; +function requiresWorkaround(node: AST.NumberLiteral): boolean; +function requiresWorkaround(node: AST.MustacheStatement | AST.NumberLiteral): boolean { + if (node.type === 'MustacheStatement' && node.path.type === 'NumberLiteral') { + return requiresWorkaround(node.path); + } else if (node.type === 'NumberLiteral') { + return isInteger(node.value) && isOverflowing(node.value); + } else { + return false; + } +} + +// Number.isInteger polyfill +function isInteger(value: number): boolean { + return isFinite(value) && Math.floor(value) === value; +} + +function isOverflowing(value: number): boolean { + return value >= 2 ** 28; +}