Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

format and test scripts generate SyntaxErrors #5

Open
sc0ttwad3 opened this issue Mar 20, 2018 · 23 comments
Open

format and test scripts generate SyntaxErrors #5

sc0ttwad3 opened this issue Mar 20, 2018 · 23 comments
Labels
bug Something isn't working help wanted Extra attention is needed
Projects

Comments

@sc0ttwad3
Copy link
Contributor

Seems reproducible with any new noderize app create, even after successful build/watch, initially running...

yarn format generates a SyntaxError via @noderize\scripts\node_modules\.bin\prettier and args handling?
yarn test generates a SyntaxError via @noderize\scripts\node_modules\.bin\jest and args handling?

λ yarn format
yarn run v1.5.1
$ noderize-scripts format
[INFO] Formatting...
c:\projects\typescript\first-noderize\node_modules\@noderize\scripts\node_modules\.bin\prettier:2
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")
          ^^^^^^^

SyntaxError: missing ) after argument list
    at new Script (vm.js:51:7)
    at createScript (vm.js:136:10)
    at Object.runInThisContext (vm.js:197:10)
    at Module._compile (module.js:613:28)
    at Object.Module._extensions..js (module.js:660:10)
    at Module.load (module.js:561:32)
    at tryModuleLoad (module.js:501:12)
    at Function.Module._load (module.js:493:3)
    at Function.Module.runMain (module.js:690:10)
    at startup (bootstrap_node.js:194:16)
[WARN] Done formatting!
Done in 1.67s.
λ yarn test
yarn run v1.5.1
$ noderize-scripts test
[INFO] Testing...
The system cannot find the path specified.
c:\projects\typescript\first-noderize\node_modules\@noderize\scripts\node_modules\.bin\jest:2
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")
          ^^^^^^^

SyntaxError: missing ) after argument list
    at new Script (vm.js:51:7)
    at createScript (vm.js:136:10)
    at Object.runInThisContext (vm.js:197:10)
    at Module._compile (module.js:613:28)
    at Object.Module._extensions..js (module.js:660:10)
    at Module.load (module.js:561:32)
    at tryModuleLoad (module.js:501:12)
    at Function.Module._load (module.js:493:3)
    at Function.Module.runMain (module.js:690:10)
    at startup (bootstrap_node.js:194:16)
Done in 1.75s.
@Cretezy
Copy link
Owner

Cretezy commented Mar 20, 2018

This seems to be a Windows-only problem.

This is odd since basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')") never appears in Noderize itself, only when calling other scripts (Prettier and Jest) using child_process.fork.

Could you provide more information on your set up?

@Cretezy Cretezy added the bug Something isn't working label Mar 20, 2018
@sc0ttwad3
Copy link
Contributor Author

Your right it's the windows shell lacking shebang support. Is it that problem where yarn/npm ends up treating/running the shell script as javascript?

It looks like create-react-app and create-react-app-typescript are utilizing node-cross-spawn to handle this issue, but that's just a quick look. I'd have to look deeper into what they're doing.

Pretty standard Windows 10 latest build, lots of ram, fast SSDs, on this Spectre x360 laptop. Specific environment I'm using...

λ cmd --version
Microsoft Windows [Version 10.0.17123.1]
(c) 2017 Microsoft Corporation. All rights reserved.

yarn versions v1.5.1
{ yarn: '1.5.1',
  myapp: '0.1.0',
  http_parser: '2.7.0',
  node: '9.8.0',
  v8: '6.2.414.46-node.21',
  uv: '1.19.2',
  zlib: '1.2.11',
  ares: '1.13.0',
  modules: '59',
  nghttp2: '1.29.0',
  napi: '2',
  openssl: '1.0.2n',
  icu: '60.2',
  unicode: '10.0',
  cldr: '32.0.1',
  tz: '2017c' }

@Cretezy
Copy link
Owner

Cretezy commented Mar 20, 2018

Ah, mimssing shebang support makes lots of sense. I'll work on a fix, give me a few minutes 👍

@sc0ttwad3
Copy link
Contributor Author

Sure thing.

Just reading the code/docs for node-cross-spawn. Specifically mentions node problems when using spawn on Windows: "Has an issue with command shims (files in node_modules/.bin/), where arguments with quotes and parenthesis would result in invalid syntax error". Looks promising.

@Cretezy
Copy link
Owner

Cretezy commented Mar 20, 2018

0f6a90f

Waiting for tests to pass then publishing as v0.4.4.

@Cretezy
Copy link
Owner

Cretezy commented Mar 20, 2018

Published as v0.4.4, thank you for the report and the library suggestion!

@Cretezy Cretezy closed this as completed Mar 20, 2018
@sc0ttwad3
Copy link
Contributor Author

I tried out 0.4.5 this afternoon (nice work) and can confirm all scripts work without any issues on linux systems for me.

format is still problematic on Widows and again giving the usual syntax error, same culprit, I suspect, the spawn issues node has on windows. Looks like cross-spawn is there now.

Some typo maybe in the format script? I'll take a quick look...

λ yarn format
yarn run v1.5.1
$ noderize-scripts format
[INFO] Formatting...
c:\projects\typescript\myapp\node_modules\@noderize\scripts\node_modules\.bin\prettier:2
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")
          ^^^^^^^

SyntaxError: missing ) after argument list
    at new Script (vm.js:51:7)
    at createScript (vm.js:136:10)
    at Object.runInThisContext (vm.js:197:10)
    at Module._compile (module.js:613:28)
    at Object.Module._extensions..js (module.js:660:10)
    at Module.load (module.js:561:32)
    at tryModuleLoad (module.js:501:12)
    at Function.Module._load (module.js:493:3)
    at Function.Module.runMain (module.js:690:10)
    at startup (bootstrap_node.js:194:16)
[WARN] Done formatting!
Done in 1.94s.

@Cretezy
Copy link
Owner

Cretezy commented Mar 21, 2018

Reopening this issue. Does this also happen on the test script?

@Cretezy Cretezy reopened this Mar 21, 2018
@Cretezy
Copy link
Owner

Cretezy commented Mar 21, 2018

Related issues:

Can you try running (from your base directory) node node_modules\@noderize\scripts\node_modules\.bin\prettier and seeing if it works.

@sc0ttwad3
Copy link
Contributor Author

Sure. I checked and prettier works fine...

λ node_modules\@noderize\scripts\node_modules\.bin\prettier --version
1.11.1

In another noderize generated app, I'm using a quick work around. I've added a script to the noderize generated app's package.json file, called format:js to invoke prettier:

"format:js": "prettier --trailing-comma es5 --single-quote --write \"./src/**/*.js\"",

It's manual, yarn format:js but prettier runs correctly even with configuration command-line options (I haven't checked alternatively using a .prettier file yet). Useful for me as a way to check/test the results of different options at the moment.

@sc0ttwad3
Copy link
Contributor Author

The test script seems to have the same issue. Syntax error when run as yarn test. However, it too, runs correctly with a fully qualified path:

c:\projects\typescript\thirdapp
λ node_modules\@noderize\scripts\node_modules\.bin\jest --version
v22.4.2

c:\projects\typescript\thirdapp
λ node_modules\@noderize\scripts\node_modules\.bin\jest
No tests found
In C:\projects\typescript\thirdapp
  3 files checked.
  testMatch: **/__tests__/**/*.js?(x),**/?(*.)(spec|test).js?(x) - 0 matches
  testPathIgnorePatterns: \\node_modules\\ - 3 matches
Pattern:  - 0 matches

c:\projects\typescript\thirdapp
λ yarn test
yarn run v1.5.1
$ noderize-scripts test
[INFO] Testing...
The system cannot find the path specified.
c:\projects\typescript\thirdapp\node_modules\@noderize\scripts\node_modules\.bin\jest:2
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")
          ^^^^^^^

SyntaxError: missing ) after argument list
    at new Script (vm.js:51:7)
    at createScript (vm.js:136:10)
    at Object.runInThisContext (vm.js:197:10)
    at Module._compile (module.js:613:28)
    at Object.Module._extensions..js (module.js:660:10)
    at Module.load (module.js:561:32)
    at tryModuleLoad (module.js:501:12)
    at Function.Module._load (module.js:493:3)
    at Function.Module.runMain (module.js:690:10)
    at startup (bootstrap_node.js:194:16)
Done in 2.28s.

@sc0ttwad3
Copy link
Contributor Author

Prettier handling command-line arguments on Windows from npm/yarn scripts has given me problems in the past. For some reason it seems to frequently not operate the way it does when called manually (more so than other tools). Are you seeing the same issues on Windows? Getting the same misleading SyntaxError output? BTW, the scripts are fine when running under bash in the Windows Subsystem for Linux (wsl) configured for Ubuntu as well, I forgot to mention.

Anyway, I've started investigating the ways people have modified react-scripts for Typescript and I have time the next couple of days for learning more about this area and cross-platform issues in general.

I've also been looking at some clever code in the typescript-starter project for handling command-line configuration pre-generation, and importantly, like this one, the option of using TypeScript or not.

But I'm not crazy about question answers instead of options on the command-line when configuring for the output project generation (like yeoman generators do) so I still like the approach of react-scripts like in this project. I'm looking at customized versions like custom-react-scripts-ts as well.

@Cretezy
Copy link
Owner

Cretezy commented Mar 22, 2018

This specific bug is pretty weird. I'm not very well versed in Windows, so would need to re-setup my Windows workstation to try to debug this. If anyone has any experience with this, please let me know, and we can work together on fixing this!

@sc0ttwad3
Copy link
Contributor Author

I have time today and tomorrow to look into it. and can let you know if I find a resolution.

@sc0ttwad3
Copy link
Contributor Author

The core issue seems to come down to npm assuming the command passed to it is a JS file--somewhere in the complicated script calling script decomposition into commands and args at work here--this is however not true on Windows where npm wraps bin files in a .cmd file.

Since npm can not parse .cmd files--where our SyntaxError: issue is coming from--you need to reference the bin file manually in the package.json scripts section as part of a script.

And in order to use this cross platform (i.e. Linux, Mac and Windows), you need to change the path from the form:

node_modules/.bin/<some_name>

to the full location and adding the .js file extension:

node_modules/<some_name>/bin/<some_name>.js

At least that's my current understanding of the issue.

@Cretezy
Copy link
Owner

Cretezy commented Mar 24, 2018 via email

@sc0ttwad3
Copy link
Contributor Author

What setup do you recommend for running a development version of Noderize locally to work with? I'm having a problem with setting up a good debug workflow. Currently, I'm jumping through too many hoops and manual operations.

And then how do you get yarn create noderize new-app to use the local development version of noderize to test your changes?

@Cretezy
Copy link
Owner

Cretezy commented Mar 24, 2018

I would say clone this main repo, go in scripts and run yarn bootstrap, then you can "test" on the 'create` package (so build scripts, and run the scripts in create).

To recap:

  • clone & cd
  • yarn
  • cd into packages/scripts, yarn bootstrap
  • do changes, yarn build (you can actually watch too)
  • cd in packages/create, yarn format or whichever command you want to test

I definitely need to make a CONTRIBUTING.md file. Will do soon!

@sc0ttwad3
Copy link
Contributor Author

Thanks! I'll be able to look into this again tonight.

@sc0ttwad3
Copy link
Contributor Author

The yarn bootstrap step fails for me. The solution is probably right in front of me and I'm just not seeing it. But, I can't seem to find a bootstrap entry in the packages/scripts directory package.json file, for yarn to run, or an entry anywhere really. I've search the entire project repo. Then I even searched, as a remote possibility, for a bootstrap.js script buried somewhere, none found as expected, however.

@Cretezy
Copy link
Owner

Cretezy commented Mar 27, 2018

My bad, try prepack

@Cretezy
Copy link
Owner

Cretezy commented Apr 1, 2018

@sc0ttwad3 if you are still working on this, I've created a CONTRIBUTING.md to help get started.

@Cretezy Cretezy added the help wanted Extra attention is needed label Apr 1, 2018
@sc0ttwad3
Copy link
Contributor Author

@Cretezy Thanks. Havent had as much time for anything past work, but with prepack working, Im looking at a couple new things I ran across for the cross platform issues. I get anywhere with it, Ill let you know.

@Cretezy Cretezy added this to To do in v1.0.0 May 27, 2018
gbuktenica pushed a commit to gbuktenica/Unicode-Substitutions that referenced this issue Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
v1.0.0
  
To do
Development

No branches or pull requests

2 participants