nodent icon indicating copy to clipboard operation
nodent copied to clipboard

Nodent (and fast-async) break in Meteor

Open trusktr opened this issue 8 years ago • 6 comments

I'm running code in Meteor, and I get

Uncaught SyntaxError: Unexpected token )

from the processIncludes function.

This is because Meteor appends a commented line number to all source code, even from node_modules, so for example

function foo() {
  blah()
}

becomes

function foo() { // 1
  blah()         // 2
}                // 3

When processIncludes concatenates the lines of the input, the comments cause the remainder of the function to be commented out, for example:

function foo() { // 1 blah()         // 2 }                // 3

See here for more details: https://github.com/meteor/meteor/issues/9160

trusktr avatar Oct 09 '17 07:10 trusktr

On another note, using new Function might not be a good idea, because with certain Content Security Policy headers present in a web app, the browser will block eval and similar mechanisms from being usable, which will cause processIncludes to fail also.

trusktr avatar Oct 09 '17 07:10 trusktr

For now, I can get by with compiler: {promises: true, noRuntime: true}, useRuntimeModule: false for fast-async, but I thought that compiler: {promises: false, noRuntime: false}, useRuntimeModule: true would be better, but in the latter case I get the above issue.

There must be a better way to put things together for nodent-runtime without string source mangling at runtime?

trusktr avatar Oct 09 '17 07:10 trusktr

What's the best fast-async config for IE 11? From my limited knowledge, it seems like compiler: {promises: false, noRuntime: false}, useRuntimeModule: true results in the smallest code with the runtime being required/imported by the files.

trusktr avatar Oct 09 '17 21:10 trusktr

I don't have a planned fix for this right now. The processIncludes routine is not very clever, but works under all versions of node without issue. It could be replaced with a pre-build phase, but this would involve a much deeper dependency tree (for webpack or rollup or similar) which I don't want to introduce.

If you want to submit a PR that fixes processIncludes in this case, or makes it's use optional via a flag/option, I'm happy to review and potentially ship.

matAtWork avatar Oct 12 '17 10:10 matAtWork

Thinking about this over the weekend, you could try replacing the first regex at https://github.com/MatAtBread/nodent-runtime/blob/master/runtime.js#L35 with \(\/\*[^*]*\*\/|\/\/[^\n]*)\

TBH, I don't have the same setup as you, so I don't have a ready test-case to hand. However, if you pull (or fork) the nodent-runtime repo, npm i, make that change and then install into your project locally (e.g npm i ../nodent-runtime) you should be able to tell if it works. If it does, submit a PR and I'll test & ship the update to npm.

matAtWork avatar Oct 16 '17 13:10 matAtWork

I'll be getting back to this soon!

trusktr avatar Jan 04 '19 08:01 trusktr