uber / bufrw

Buffer Reading and Writing
MIT License
32 stars 19 forks source link

Server-side bundling fails due to octals in ansi-color #63

Open shannonmoeller opened 5 years ago

shannonmoeller commented 5 years ago

When trying to use bufrw (via thriftrw) in an environment where server side code is bundled with Babel and Webpack, the build process throws an error related to using Octal literals in strict mode:

error: ./node_modules/ansi-color/lib/ansi-color.js
Module build failed (from ./node_modules/fusion-cli/build/loaders/babel-loader.js):
SyntaxError: /var/tmp/project/node_modules/ansi-color/lib/ansi-color.js: Octal literal in strict mode (35:18)

  33 |   var ansi_str = "";
  34 |   for(var i=0, attr; attr = color_attrs[i]; i++) {
> 35 |     ansi_str += "\033[" + ANSI_CODES[attr] + "m";
     |                   ^
  36 |   }
  37 |   ansi_str += str + "\033[" + ANSI_CODES["off"] + "m";
  38 |   return ansi_str;
    at Parser.raise (/var/tmp/project/node_modules/@babel/parser/lib/index.js:6322:17)
    at Parser.readEscapedChar (/var/tmp/project/node_modules/@babel/parser/lib/index.js:7418:20)
    at Parser.readString (/var/tmp/project/node_modules/@babel/parser/lib/index.js:7265:21)
    at Parser.getTokenFromCode (/var/tmp/project/node_modules/@babel/parser/lib/index.js:6950:14)
    at Parser.nextToken (/var/tmp/project/node_modules/@babel/parser/lib/index.js:6520:12)
    at Parser.next (/var/tmp/project/node_modules/@babel/parser/lib/index.js:6460:10)
    at Parser.parseMaybeAssign (/var/tmp/project/node_modules/@babel/parser/lib/index.js:8210:12)
    at Parser.parseExpression (/var/tmp/project/node_modules/@babel/parser/lib/index.js:8120:23)
    at Parser.parseStatementContent (/var/tmp/project/node_modules/@babel/parser/lib/index.js:9892:23)
    at Parser.parseStatement (/var/tmp/project/node_modules/@babel/parser/lib/index.js:9763:17)
    at Parser.parseBlockOrModuleBlockBody (/var/tmp/project/node_modules/@babel/parser/lib/index.js:10340:25)
    at Parser.parseBlockBody (/var/tmp/project/node_modules/@babel/parser/lib/index.js:10327:10)
    at Parser.parseBlock (/var/tmp/project/node_modules/@babel/parser/lib/index.js:10311:10)
    at Parser.parseStatementContent (/var/tmp/project/node_modules/@babel/parser/lib/index.js:9839:21)
    at Parser.parseStatement (/var/tmp/project/node_modules/@babel/parser/lib/index.js:9763:17)
    at node.body.withTopicForbiddingContext (/var/tmp/project/node_modules/@babel/parser/lib/index.js:10374:60)
 @ ./node_modules/bufrw/error_highlighter.js 22:12-33
 @ ./node_modules/bufrw/interface.js
 @ ./node_modules/bufrw/index.js
 @ ./node_modules/thriftrw/index.js
 @ ./src/server/lib/thrift-http.js
 @ ./src/server/app.js

Are you open to a PR replacing the use of ansi-color with ansi-colors or chalk?

kriskowal commented 5 years ago

Please reopen if this continues to be a problem. My impression is that the underlying problem with these tools has been addressed.

johnmaia commented 5 years ago

@kriskowal this is still an issue with version 1.3.0.

For some reason the changes from #64 were reverted. And there is also the hexer@1.5.0 dependency that still uses ansi-color.

kriskowal commented 5 years ago

Thank you for letting us know. I’ve reopened the issue.

On Wed, Nov 13, 2019 at 4:54 AM John Maia notifications@github.com wrote:

@kriskowal https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_kriskowal&d=DwMCaQ&c=r2dcLCtU9q6n0vrtnDw9vg&r=-mHabCODSL9xa3I-KsqWgQ&m=2EBph3IOiwP2FsjJlQmA_Eibx-bfQVJk5UT2QwAWmXI&s=S2rbAL2ENT3HZUS0J9XXf9GGofiaIm9wHyySNMxOGmk&e= this is still an issue with version 1.3.0.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_uber_bufrw_issues_63-3Femail-5Fsource-3Dnotifications-26email-5Ftoken-3DAAAOXBUYFLKIQURL2EKVENDQTP2HPA5CNFSM4HJUYTU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOED6BHYY-23issuecomment-2D553391075&d=DwMCaQ&c=r2dcLCtU9q6n0vrtnDw9vg&r=-mHabCODSL9xa3I-KsqWgQ&m=2EBph3IOiwP2FsjJlQmA_Eibx-bfQVJk5UT2QwAWmXI&s=b-XQDsRKrlnmqK1ehFpF3wTehR7RZ3OiPT423pDf6Wg&e=, or unsubscribe https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AAAOXBTA4SBEIEUMLTQ6ASDQTP2HPANCNFSM4HJUYTUQ&d=DwMCaQ&c=r2dcLCtU9q6n0vrtnDw9vg&r=-mHabCODSL9xa3I-KsqWgQ&m=2EBph3IOiwP2FsjJlQmA_Eibx-bfQVJk5UT2QwAWmXI&s=RBg2xP81D1xTOrQG0zYQt0Q-vPH255uvv4inAycYfic&e= .

johnmaia commented 5 years ago

Thanks @kriskowal! This issue appeared to us because we have an application that has jaeger-client@3.16.0 as a dependency and jest@24.8.0 as a dev-dependency and when we attempt to run jest --coverage we get the following error:

Test suite failed to run

    SyntaxError: /Users/johnmaia/Projects/foobar/node_modules/ansi-color/lib/ansi-color.js: Octal literal in strict mode (35:18)

      33 |   var ansi_str = "";
      34 |   for(var i=0, attr; attr = color_attrs[i]; i++) {
    > 35 |     ansi_str += "\033[" + ANSI_CODES[attr] + "m";
         |                   ^
      36 |   }
      37 |   ansi_str += str + "\033[" + ANSI_CODES["off"] + "m";
      38 |   return ansi_str;

      at Parser.raise (node_modules/@babel/parser/lib/index.js:6420:17)
      at Parser.readEscapedChar (node_modules/@babel/parser/lib/index.js:7542:20)
      at Parser.readString (node_modules/@babel/parser/lib/index.js:7389:21)
      at Parser.getTokenFromCode (node_modules/@babel/parser/lib/index.js:7057:14)
      at Parser.nextToken (node_modules/@babel/parser/lib/index.js:6630:12)
      at Parser.next (node_modules/@babel/parser/lib/index.js:6559:10)
      at Parser.parseMaybeAssign (node_modules/@babel/parser/lib/index.js:8361:12)
      at Parser.parseExpression (node_modules/@babel/parser/lib/index.js:8275:23)
      at Parser.parseStatementContent (node_modules/@babel/parser/lib/index.js:10138:23)
      at Parser.parseStatement (node_modules/@babel/parser/lib/index.js:10009:17)

If we use an older version of jest like version 21.1.0 this is no longer an issue. We've tried to find a babel plugin that would handle this issue, given that we don't wish to downgrade the version of jest on all of our projects, but we had no success.

kriskowal commented 4 years ago

The bottom line is that this is valid JavaScript and the bug is presumably in Babel. The line beneath the bottom line is that this could be addressed by using hex instead of octal in ansi-color. Perhaps attach related issues from those projects. I believe we would prefer to resolve this issue with changes to these dependencies.

Issue #64 was closed by the author who found a work-around. It was not landed nor reverted. If we had reviewed it, would would have balked at absorbing an adapter for ansi-colors. We would like to keep this library’s scope tight and work with our ecosystem to resolve issues closer to their source.

Thanks for tracking the issue. We are not planning changes, but will review suggestions. We are open to uprevving our dependencies as long as we preserve support for Node.js 0.10.

Raynos commented 4 years ago

I recently choose to try server-side bundling for some projects.

I would recommend parcel

parcel build index.js --bundle-node-modules --target node --no-minify --log-level 4

This does a great job of taking a simple program that you would run with node index.js and creating a single bundle in dist/index.js that can be run without node_modules or npm install with node dist/index.js

Raynos commented 4 years ago

I just tried parcel with bufrw and it generates a working bundle ; i'd second that this is an issue with babel and webpack ;

shannonmoeller commented 4 years ago

The bottom line is that this is valid JavaScript

Yes, but non-normative and discouraged JavaScript.

would have balked at absorbing an adapter for ansi-colors

As you should have. It's why I also opened loopj/commonjs-ansi-color#14.

Ricardo-Li commented 4 years ago

@johnmaia Thanks for your workaround!

when I run jest --coverage, I found the same issue on my project. Besides using an old version of jest, do you found any other solutions?

verygoodsoftwarenotvirus commented 3 years ago

Hi, I have this issue while trying to use OpenTelemetry code in my application. I just want my perfectly functioning code to work and not be hobbled by dependencies I didn't install.

johnmaia commented 3 years ago

@Ricardo-Li we ended up dropping the package that depended on bufrw so other than downgrading jest I don't have a better solution.

Raynos commented 3 years ago

Have you tried opening a bug report on the jest project ?

shannonmoeller commented 3 years ago

It's not a jest issue. I don't understand the resistance to fixing this. It's a small change and I already did the hard part if an adapter is okay (#64). Otherwise using a different dependency would also fix it.

artdent commented 1 year ago

If anyone is reaching this bug due to OpenTelemetry or jest -- specifically, due the OpenTelemetry SDK transitively depending on this project via jaeger-client -- the fix is to make sure that the node_modules directory is properly excluded from jest coverage computation (via the coveragePathIgnorePatterns or collectCoverageFrom options).

Insem commented 1 year ago

If anyone is reaching this bug due to OpenTelemetry or jest -- specifically, due the OpenTelemetry SDK transitively depending on this project via jaeger-client -- the fix is to make sure that the node_modules directory is properly excluded from jest coverage computation (via the coveragePathIgnorePatterns or collectCoverageFrom options).

Also check does you collect coverage from .js files. I had the same issue, ignoring .js files helped me. I use typescript instead.