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

Template string literals in node_modules are mutated by injected line numbers. #9160

Closed
trusktr opened this Issue Oct 1, 2017 · 8 comments

Comments

Projects
None yet
4 participants
@trusktr
Contributor

trusktr commented Oct 1, 2017

This bug report should include:

  • The version of Meteor showing the problem. 1.5.1
  • The last version of Meteor where the problem did not occur (if applicable): don't know
  • The operating system you're running Meteor on.: Linux
  • The expected behavior.: Meteor should not put line number on multi-line template literals.
  • The actual behavior. Meteor puts line numbers on on each line of a multi-line template string, effectively destroying the value of the string.
  • A simple reproduction!
    (example: A GitHub repository that anyone can clone to observe the problem.)

I don't have a reproduction, but basically, if we have code like the following in node_modules:

const string = `
  one
  two
  three
`

then Meteor is putting line numbers on it like this:

const string = `              // 1
  one                         // 2
  two                         // 3
  three                       // 4
`                             // 5

This can break applications (obviously). For example, I was using a multi-line template literal to create the content for a style attribute of an element, but the style was silently failing with no indication of the problem until I logged the string (after hunting it down for a good while).

@trusktr

This comment has been minimized.

Contributor

trusktr commented Oct 1, 2017

This problem started happening after I stopped transpiling template strings in my NPM package because I'm supporting newer browsers.

@trusktr trusktr changed the title from Template literals that aren't transpiled are destroyed by injected line numbers. to Template string literals in node_modules are mutated by injected line numbers. Oct 1, 2017

@trusktr

This comment has been minimized.

Contributor

trusktr commented Oct 1, 2017

In some cases a workaround is to put the template string on one line (in other cases, spaces might matter and this solution won't work). For example, in my case:

var cssMatrixString = `matrix3d( ${ domMatrix.m11 }, ${ domMatrix.m12 }, ${ domMatrix.m13 }, ${ domMatrix.m14 }, ${ domMatrix.m21 }, ${ domMatrix.m22 }, ${ domMatrix.m23 }, ${ domMatrix.m24 }, ${ domMatrix.m31 }, ${ domMatrix.m32 }, ${ domMatrix.m33 }, ${ domMatrix.m34 }, ${ domMatrix.m41 }, ${ domMatrix.m42 }, ${ domMatrix.m43 }, ${ domMatrix.m44 })`;
@klaussner

This comment has been minimized.

Collaborator

klaussner commented Oct 1, 2017

@benjamn

This comment has been minimized.

Member

benjamn commented Oct 3, 2017

Looks like this commit didn't go far enough, since the last sentence of the commit message is false for modules in node_modules: c7690ae

@trusktr

This comment has been minimized.

Contributor

trusktr commented Oct 9, 2017

This breaks the fast-async Babel plugin (breaks Nodent), because Nodent uses toSource() on some functions in its implementation.

I was trying to transpile async functions in my NPM package, but then it broke when I ran it in a Meteor app.


I don't think Meteor should be line numbering anything in node_modules, or maybe not even at all. I think source maps are enough, and that focus can be on getting those polished instead.

@trusktr

This comment has been minimized.

Contributor

trusktr commented Oct 9, 2017

Here's example code breaking fast-async:

(function(self,catcher
/*``*/) {
return function $asyncbind(self,catcher) { // 43 "use strict"; // 44 if (!Function.prototype.$asyncbind) { // 45 Object.defineProperty(Function.prototype,"$asyncbind",{value:$asyncbind,enumerable:false,configurable:true,writable:true}) ; } // 47 // 48 if (!$asyncbind.trampoline) { // 49 $asyncbind.trampoline = function trampoline(t,x,s,e,u){ // 50 return function b(q) { // 51 while (q) { // 52 if (q.then) { // 53 q = q.then(b, e) ; // 54 return u?undefined:q; // 55 } // 56 try { // 57 if (q.pop) { // 58 if (q.length) // 59 return q.pop() ? x.call(t) : q; // 60 q = s; // 61 } else // 62 q = q.call(t) // 63 } catch (r) { // 64 return e(r); // 65 } // 66 } // 67 } // 68 }; // 69 } // 70 if (!$asyncbind.LazyThenable) { // 71 $asyncbind.LazyThenable = function () { // 1 function isThenable(obj) { // 2 return obj && (obj instanceof Object) && typeof obj.then==="function"; // 3 } // 4 // 5 function resolution(p,r,how) { // 6 try { // 7 // 8 var x = how ? how(r):r ; // 9 // 10 if (p===x) // 11 return p.reject(new TypeError("Promise resolution loop")) ; // 12 // 13 if (isThenable(x)) { // 14 // 15 x.then(function(y){ // 16 resolution(p,y); // 17 },function(e){ // 18 p.reject(e) // 19 }) ; // 20 } else { // 21 p.resolve(x) ; // 22 } // 23 } catch (ex) { // 24 // 25 p.reject(ex) ; // 26 } // 27 } // 28 // 29 function Chained() {}; // 30 Chained.prototype = { // 31 resolve:_unchained, // 32 reject:_unchained, // 33 then:thenChain // 34 }; // 35 function _unchained(v){} // 36 function thenChain(res,rej){ // 37 this.resolve = res; // 38 this.reject = rej; // 39 } // 40 // 41 function then(res,rej){ // 42 var chain = new Chained() ; // 43 try { // 44 this._resolver(function(value) { // 45 return isThenable(value) ? value.then(res,rej) : resolution(chain,value,res); // 46 },function(ex) { // 47 resolution(chain,ex,rej) ; // 48 }) ; // 49 } catch (ex) { // 50 resolution(chain,ex,rej); // 51 } // 52 return chain ; // 53 } // 54 // 55 function Thenable(resolver) { // 56 this._resolver = resolver ; // 57 this.then = then ; // 58 }; // 59 // 60 Thenable.resolve = function(v){ // 61 return Thenable.isThenable(v) ? v : {then:function(resolve){return resolve(v)}}; // 62 }; // 63 // 64 Thenable.isThenable = isThenable ; // 65 // 66 return Thenable ; // 67 }(); // 72 $asyncbind.EagerThenable = $asyncbind.Thenable = ($asyncbind.EagerThenableFactory = function (tick){ // 8 tick = tick || (typeof process==="object" && process.nextTick) || (typeof setImmediate==="function" && setImmediate) || function(f){setTimeout(f,0)}; var soon = (function () { // 10 var fq = [], fqStart = 0, bufferSize = 1024; // 11 function callQueue() { // 12 while (fq.length - fqStart) { // 13 try { fq[fqStart]() } catch(ex) { } // 14 fq[fqStart++] = undefined; // 15 if (fqStart === bufferSize) { // 16 fq.splice(0, bufferSize); // 17 fqStart = 0; // 18 } // 19 } // 20 } // 21 // 22 return function (fn) { // 23 fq.push(fn); // 24 if (fq.length - fqStart === 1) // 25 tick(callQueue); // 26 }; // 27 })(); // 28 // 29 function Zousan(func) { // 30 if (func) { // 31 var me = this; // 32 func(function (arg) { // 33 me.resolve(arg); // 34 }, function (arg) { // 35 me.reject(arg); // 36 }); // 37 } // 38 } // 39 // 40 Zousan.prototype = { // 41 resolve: function (value) { // 42 if (this.state !== undefined) // 43 return; // 44 if (value === this) // 45 return this.reject(new TypeError("Attempt to resolve promise with self")); // 46 var me = this; // 47 if (value && (typeof value === "function" || typeof value === "object")) { // 48 try { // 49 var first = 0; // 50 var then = value.then; // 51 if (typeof then === "function") { // 52 then.call(value, function (ra) { // 53 if (!first++) { // 54 me.resolve(ra); // 55 } // 56 }, function (rr) { // 57 if (!first++) { // 58 me.reject(rr); // 59 } // 60 }); // 61 return; // 62 } // 63 } catch (e) { // 64 if (!first) // 65 this.reject(e); // 66 return; // 67 } // 68 } // 69 this.state = STATE_FULFILLED; // 70 this.v = value; // 71 if (me.c) // 72 soon(function () { // 73 for (var n = 0, l = me.c.length;n < l; n++) // 74 STATE_FULFILLED(me.c[n], value); // 75 }); // 76 }, // 77 reject: function (reason) { // 78 if (this.state !== undefined) // 79 return; // 80 this.state = STATE_REJECTED; // 81 this.v = reason; // 82 var clients = this.c; // 83 if (clients) // 84 soon(function () { // 85 for (var n = 0, l = clients.length;n < l; n++) // 86 STATE_REJECTED(clients[n], reason); // 87 }); // 88 }, // 89 then: function (onF, onR) { // 90 var p = new Zousan(); // 91 var client = { // 92 y: onF, // 93 n: onR, // 94 p: p // 95 }; // 96 if (this.state === undefined) { // 97 if (this.c) // 98 this.c.push(client); // 99 else // 100 this.c = [client]; // 101 } else { // 102 var s = this.state, a = this.v; // 103 soon(function () { // 104 s(client, a); // 105 }); // 106 } // 107 return p; // 108 } // 109 }; // 110 // 111 function STATE_FULFILLED(c, arg) { // 112 if (typeof c.y === "function") { // 113 try { // 114 var yret = c.y.call(undefined, arg); // 115 c.p.resolve(yret); // 116 } catch (err) { // 117 c.p.reject(err); // 118 } // 119 } else // 120 c.p.resolve(arg); // 121 } // 122 // 123 function STATE_REJECTED(c, reason) { // 124 if (typeof c.n === "function") { // 125 try { // 126 var yret = c.n.call(undefined, reason); // 127 c.p.resolve(yret); // 128 } catch (err) { // 129 c.p.reject(err); // 130 } // 131 } else // 132 c.p.reject(reason); // 133 } // 134 // 135 Zousan.resolve = function (val) { // 136 if (val && (val instanceof Zousan)) // 137 return val ; // 138 var z = new Zousan(); // 139 z.resolve(val); // 140 return z; // 141 }; // 142 Zousan.reject = function (err) { // 143 if (err && (err instanceof Zousan)) // 144 return err ; // 145 var z = new Zousan(); // 146 z.reject(err); // 147 return z; // 148 }; // 149 // 150 Zousan.version = "2.3.3-nodent" ; // 151 return Zousan ; // 152 })(); // 73 } // 74 // 75 var resolver = this; // 76 switch (catcher) { // 77 case true: // 78 return new ($asyncbind.Thenable)(boundThen); // 79 case 0: // 80 return new ($asyncbind.LazyThenable)(boundThen); // 81 case undefined: // 82 // 83 boundThen.then = boundThen ; // 84 return boundThen ; // 85 default: // 86 return function(){ // 87 try { // 88 return resolver.apply(self,arguments); // 89 } catch(ex) { // 90 return catcher(ex); // 91 } // 92 } // 93 } // 94 function boundThen() { // 95 return resolver.apply(self,arguments); // 96 } // 97 }
})
@trusktr

This comment has been minimized.

Contributor

trusktr commented Oct 9, 2017

Not sure how/why it's on a single line like that.

Nodent's processIncludes function is concatenating lines, probably under the assumption that it's source code is not altered.

@mattmccutchen

This comment has been minimized.

Contributor

mattmccutchen commented Nov 2, 2017

It looks like this problem may affect the output of build plugins as well as node_modules. I saw it with template literals in TypeScript files in my app when I tried upgrading to Meteor 1.6 and switching my custom TypeScript build plugin to target ES6 instead of ES5. I don't expect much help with this since I know my configuration is discouraged for several reasons even if not officially unsupported (the build plugin is using the deprecated Package.registerSourceHandler and the modules package isn't enabled in the app, so the build system takes significantly different code paths than normal), though it was interesting that the problem affected both client and server.

However, I also reproduced the problem (on the client only) using barbatus:typescript with tsconfig.json set to target ES6. Is this representative of a supported configuration?

benjamn added a commit that referenced this issue Nov 8, 2017

Stop adding line number comments to linked JavaScript files.
Several years ago, before all major browsers supported source maps, we
felt it was important to provide line number information in generated
files using end-of-line comments like "// 123\n".

Adding all these comments was always slower than leaving the code
unmodified, and recently they have begun interacting badly with certain
newer ECMAScript syntax, such as multi-line template strings (#9160).

Since source maps are well supported in most browsers that developers are
likely to be using for development, and the line number comments are now
causing substantive problems beyond the performance cost, I think it's
time we stopped using them once and for all.

Fixes #9160.

@benjamn benjamn closed this in #9323 Nov 8, 2017

benjamn added a commit that referenced this issue Nov 8, 2017

Stop adding line number comments to linked JavaScript files. (#9323)
Several years ago, before all major browsers supported source maps, we
felt it was important to provide line number information in generated
files using end-of-line comments like "// 123\n".

Adding all these comments was always slower than leaving the code
unmodified, and recently they have begun interacting badly with certain
newer ECMAScript syntax, such as multi-line template strings (#9160).

Since source maps are well supported in most browsers that developers are
likely to be using for development, and the line number comments are now
causing substantive problems beyond the performance cost, I think it's
time we stopped using them once and for all.

Fixes #9160.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment