Mobx decorate 1.0.8 #2818

jeremy-coleman · 2021-02-18T18:16:07Z

This PR modifies mobx undecorate to spawn jscodeshift using the system path -child_process.spawn("jscodeshift", ...) instead of a relative path. The reasoning is that you should be able to reliably call a package by name if it is using a bin field in its package.json (as all node cli applications do) per the npm docs https://docs.npmjs.com/cli/v7/configuring-npm/package-json#bin .

I also added an additional help message to the cli if users pass a --help arg. jscodeshift usage will be appended to the mobx-undecorate message

further only partially relevant info:
CJS require.resolve algorithm psuedo code: https://nodejs.org/api/modules.html#modules_all_together
ESM resolve algorithm https://nodejs.org/api/esm.html#esm_resolver_algorithm (likely 0 importance)
https://docs.npmjs.com/cli/v7/commands/npx
https://docs.npmjs.com/cli/v7/commands/npm-exec

changeset-bot · 2021-02-18T18:16:09Z

🦋 Changeset detected

Latest commit: 10de240

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
mobx-undecorate	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

mweststrate

I just tried this change locally, but items from the node_modules/.bin folder don't seem to be added to the search path magically, and running node cli.js fails with /bin/sh: jscodeshift: command not found. Could you look into a more reliable fix?

jeremy-coleman · 2021-03-01T23:20:20Z

How are you running the cli locally? I just did a quick setup internally in the repo to see if it works, still everything fine here. Maybe its a windows/linux thing?

I don't want to commit the changes in the picture below, but i'll describe here.
I created another workspace with no deps except mobx-undecorate and ran "yarn workspace mobx-undecorate-tester mobx-undecorate". I did this just to test that it works for running as a standalone package, not from within the package itself. All tests pass here also, i really dont know :\

If the bin path thing really is an issue, we could just require jscodeshift and use its api directly.
Something simple like this would get the job done and could be easier to pass in proper globs to the cli.

"use strict"

var jscodeshiftRunner = require("jscodeshift/dist/Runner")
var path = require("path")


//copy/pasted Runner.js to Runner.ts and used infer parameters, so i guess these are most of the options?
//{ cpus: number; extensions: string; ignorePattern: any; ignoreConfig: any; runInBand: any; silent: any; dry: any; babel: any; verbose: any; }

function run(targetFilePaths, opts) {
    opts = {
        //verbose: false,
        //babel: true,
        //runInBand: false,
        //silent: false,
        extensions: "js,jsx,ts,tsx",
        ...opts
    }
    jscodeshiftRunner.run(require.resolve("./src/undecorate.ts"), targetFilePaths, opts)
    
}

The downside of this is figuring out how to pass nested babel options to jscodeshift , specifically the case for non-jsx types like
in the test case for

export function useAlias(): string {
    const parsedSearch = useSearch()
    return (<string>parsedSearch.alias || "").toUpperCase()
}

In practice, i would expect most people to have stopped doing this by now though.

I am also questioning, does mobx-undecorate even need to be a CLI? exporting a function people could use in a script would probably be just as easy or even easier, and if people want to use a cli , just advise them use jscodeshift directly.

mweststrate · 2021-03-02T00:54:54Z

Is codeshift perhaps on your path already, as globally installed package perhaps?

…

On Mon, 1 Mar 2021, 23:20 Jeremy, ***@***.***> wrote: How are you running the cli locally? I just did a quick setup internally in the repo to see if it works, still everything fine here. Maybe its a windows/linux thing? I don't want to commit the changes in the picture below, but i'll describe here. I created another workspace with no deps except mobx-undecorate and ran "yarn workspace mobx-undecorate-tester mobx-undecorate". I did this just to test that it works for running as a standalone package, not from within the package itself. All tests pass here also, i really dont know :\ [image: image] <https://user-images.githubusercontent.com/25135855/109571200-534c9b80-7ab9-11eb-86fd-2b376bccfef7.png> I could make a little script to run jscodeshift manually . something like this.. "use strict" var jscodeshiftRunner = require("jscodeshift/dist/Runner")var path = require("path") //copy/pasted Runner.js to Runner.ts and used infer parameters, so i guess these are most of the options?//{ cpus: number; extensions: string; ignorePattern: any; ignoreConfig: any; runInBand: any; silent: any; dry: any; babel: any; verbose: any; } function run(targetFilePaths, opts) { opts = { //verbose: false, //babel: true, //runInBand: false, //silent: false, extensions: "js,jsx,ts,tsx", ...opts } jscodeshiftRunner.run(require.resolve("./src/undecorate.ts"), targetFilePaths, opts) } — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2818 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBFJVEEYRU4GOVC7A5LTBQOMFANCNFSM4X2Y6MZQ> .

jeremy-coleman · 2021-03-02T01:15:10Z

No, I don't have it globally installed, BUT! I think changing the cli to use spawn("npx jscodeshift .. ") instead of spawn("jscodeshift ..") would actually be the proper way to do it because npx will first check the local ./bins , but spawn doesn't do that. Please try that locally , I think that is going to be the easy fix.

mweststrate · 2021-03-03T13:00:51Z

Ok did some crowd sourcing, seems the proper way to do this is to use child_process.fork, instead of .spawn, and then we should be able to use require.resolve("jscodeshift") to get the correct module path. From there we might need to append something, like 'bin/jscodeshift.js', or maybe it works out of the box. Would you mind looking into that?

jeremy-coleman · 2021-03-03T16:47:48Z

I think that will basically the same thing as it was before hehe. I will check it out tonight. The way i normally choose between cp.fork/spawn/exec is like: fork is for ipc, spawn if u need to pass through args/use a shell, and exec if you just want to use a raw string command. Dont know if thats proper but thats all that my brain is willing to take in lol. I will play with it some more tonight. Maybe could try using execa or cross-spawn packages , i see those a lot

jeremy-coleman · 2021-03-04T13:32:46Z

dear god.

I think this should fix spawning cross platform bins and also different install methods across package managers. I discovered npx installs the package as a peer to its dependencies. -_-

this bit normalizes finding the correct path

#!/usr/bin/env node
const path = require("path")
const cp = require("child_process")
const fs = require("fs")

/**
 * @example getCommandPath("jscodeshift")
 * //-> C:\Users\name\AppData\Local\npm-cache\_npx\234242somehash\node_modules\.bin\jscodeshift.cmd
 * //-> linux/path/npm-cache/_npx/234242somehash/node_modules/.bin/jscodeshift
 */
const getCommandPath = binCommand => {
  const cmd = process.platform === 'win32' ? `${binCommand}.cmd` : binCommand;
  /**
   * Normally, for executing bins from a project you would use path.resolve(__dirname, 'node_modules', '.bin', cmd) 
   * but NPX is wierd. You might think running npx mobx-undecorate installs mobx-undecorate, BUT IT DOESNT. 
   * It creates a randomly hashed folder with an unnamed package.json with mobx-undecorate as its only dependency. 
   * This causes a flattening of all peers in the same node_modules dir.
   * They probably did it this way to dedupe nested deps.
   * 
   * This following logic checks for both folder structure and platform bin file extension.
  */
  let COMMAND_PATH_SIBLING = path.resolve(__dirname, '..', '.bin', cmd)
  let COMMAND_PATH_NESTED = path.resolve(__dirname, 'node_modules', '.bin', cmd)

  var COMMAND_PATH

  if (fs.existsSync(COMMAND_PATH_NESTED)) {
    COMMAND_PATH = COMMAND_PATH_NESTED
  }
  else if (fs.existsSync(COMMAND_PATH_SIBLING)) {
    COMMAND_PATH = COMMAND_PATH_SIBLING
  }
  else {
    throw new Error("cannot find jscodeshift path")
    process.exit(0)
  }
  console.log(COMMAND_PATH)
  return COMMAND_PATH

}

I also added some arg parsing so user can do any of the following:

npx mobx-undecorate src

npx mobx-undecorate src/stores

npx mobx-undecorate src/stores/UserStore.ts

npx mobx-undecorate --dir=src

npx mobx-undecorate --dir src

npx mobx-undecorate --path src

npx mobx-undecorate --path=src

here's the logic for that

function interpret_cli_args() {

  //first 2 args of argv are the node.exe path and the path of this file.
  var USER_ARGS = process.argv.slice(2)

  /**
   * find args that dont include a "=" and set the input to the next index value in process.argv.
   * Gotta do this because process.argv is delimited by spaces, so --dir src is actually 2 separate args
   * so if an arg starts with -- and doesn't include a "=" 
   * we can just search for the arg by its --name and add 1 to the index position for lookup
   * This will return -1 if nothing is found
   */

  let arg_without_equal = USER_ARGS
    .slice()
    .filter(v => !v.includes("="))
    .findIndex(kwarg => (kwarg.includes("--dir") || kwarg.includes("--path")))
    ;

  var arg_with_equal = USER_ARGS
    .slice()
    .find(v => (v.includes("--dir=") || v.includes("--path=")))


  //use cwd as default, but will override it with user args if they exist for --dir or --path
  var PARSED_INPUT = ""

  let is_arg_directory_only = fs.existsSync(path.resolve(process.cwd(), process.argv[2]))

  if (is_arg_directory_only) {
    PARSED_INPUT = process.argv[2]
  }

  if (arg_without_equal > -1) {
    PARSED_INPUT = USER_ARGS[arg_without_equal + 1]
  }

  if (arg_with_equal) {
    PARSED_INPUT = arg_with_equal.split("=")[1]
  }

  return PARSED_INPUT

}

and here's the updated spawn command. it still falls back to process.cwd() if no user args or it cant find anything.

spawnBin("jscodeshift", [
  "--extensions=js,jsx,ts,tsx",
  ...process.argv.filter(arg => arg.startsWith("--")),
  "-t", path.join(__dirname, "src", "undecorate.ts"),

  //this is arg to tell jscodeshift the dir to transform or fallback to process.cwd()
  //originally just hard coded to process.cwd()
  path.join(process.cwd(), interpret_cli_args())
]);

I have it accepting either dir or path to do same thing, maybe unnecessary but whatever.

@mweststrate
the whole file is here , can you test it however you did before? After its good il update the pr code

#!/usr/bin/env node
const path = require("path")
const cp = require("child_process")
const fs = require("fs")

/**
 * @example getCommandPath("jscodeshift")
 * //-> C:\Users\name\AppData\Local\npm-cache\_npx\234242somehash\node_modules\.bin\jscodeshift.cmd
 * //-> linux/path/npm-cache/_npx/234242somehash/node_modules/.bin/jscodeshift
 */
const getCommandPath = binCommand => {
  const cmd = process.platform === 'win32' ? `${binCommand}.cmd` : binCommand;
  /**
   * Normally, for executing bins from a project you would use path.resolve(__dirname, 'node_modules', '.bin', cmd) 
   * but NPX is wierd. You might think running npx mobx-undecorate installs mobx-undecorate, BUT IT DOESNT. 
   * It creates a randomly hashed folder with an unnamed package.json with mobx-undecorate as its only dependency. 
   * This causes a flattening of all peers in the same node_modules dir.
   * They probably did it this way to dedupe nested deps.
   * 
   * This following logic checks for both folder structure and platform bin file extension.
  */
  let COMMAND_PATH_SIBLING = path.resolve(__dirname, '..', '.bin', cmd)
  let COMMAND_PATH_NESTED = path.resolve(__dirname, 'node_modules', '.bin', cmd)

  var COMMAND_PATH

  if (fs.existsSync(COMMAND_PATH_NESTED)) {
    COMMAND_PATH = COMMAND_PATH_NESTED
  }
  else if (fs.existsSync(COMMAND_PATH_SIBLING)) {
    COMMAND_PATH = COMMAND_PATH_SIBLING
  }
  else {
    throw new Error("cannot find jscodeshift path")
    process.exit(0)
  }
  console.log(COMMAND_PATH)
  return COMMAND_PATH

}

const spawnBin = (binCommand, args) => {
  return cp.spawn(getCommandPath(binCommand), args, {
    cwd: path.resolve(__dirname),
    stdio: 'inherit',
    shell: true
  })

}


if (process.argv.includes("--help")) {
  console.log(`[MOBX-UNDECORATE]:
  If experiencing problems, you may also install jscodeshift and mobx-undecorate locally and run 
  npx jscodeshift -t ./node_modules/mobx-undecorate/src/undecorate.ts --extensions=js,jsx,ts,tsx <directory>

[JSCODESHIFT HELP]:
`)
}


function interpret_cli_args() {

  //first 2 args of argv are the node.exe path and the path of this file.
  var USER_ARGS = process.argv.slice(2)

  /**
   * find args that dont include a "=" and set the input to the next index value in process.argv.
   * Gotta do this because process.argv is delimited by spaces, so --dir src is actually 2 separate args
   * so if an arg starts with -- and doesn't include a "=" 
   * we can just search for the arg by its --name and add 1 to the index position for lookup
   * This will return -1 if nothing is found
   */

  let arg_without_equal = USER_ARGS
    .slice()
    .filter(v => !v.includes("="))
    .findIndex(kwarg => (kwarg.includes("--dir") || kwarg.includes("--path")))
    ;

  var arg_with_equal = USER_ARGS
    .slice()
    .find(v => (v.includes("--dir=") || v.includes("--path=")))


  //use cwd as default, but will override it with user args if they exist for --dir or --path
  var PARSED_INPUT = ""

  let is_arg_directory_only = fs.existsSync(path.resolve(process.cwd(), process.argv[2]))

  if (is_arg_directory_only) {
    PARSED_INPUT = process.argv[2]
  }

  if (arg_without_equal > -1) {
    PARSED_INPUT = USER_ARGS[arg_without_equal + 1]
  }

  if (arg_with_equal) {
    PARSED_INPUT = arg_with_equal.split("=")[1]
  }

  return PARSED_INPUT

}


spawnBin("jscodeshift", [
  "--extensions=js,jsx,ts,tsx",
  ...process.argv.filter(arg => arg.startsWith("--")),
  "-t", path.join(__dirname, "src", "undecorate.ts"),
    
  //this is arg to tell jscodeshift the dir to transform or fallback to process.cwd()
  //originally just hard coded to process.cwd()
  path.join(process.cwd(), interpret_cli_args())

]);

mweststrate · 2021-03-25T11:39:45Z

Geez, sorry for the late follow up. Github dashboard showed the PR as "requested changes", while it is on "request review" state :/. I tested it and worked. Thanks!

jeremy-coleman added 2 commits February 18, 2021 13:06

modify cli to use system path for jscodeshift

3cd4a90

add changeset

bb62238

jeremy-coleman mentioned this pull request Feb 18, 2021

mobx-undecorate odd behavior #2813

Closed

mweststrate approved these changes Mar 1, 2021

View reviewed changes

mweststrate requested changes Mar 1, 2021

View reviewed changes

jeremy-coleman added 2 commits March 4, 2021 08:33

Merge branch 'main' into mobx-decorate-1.0.8

d5b963d

xplat bin logic and cli arg parsing

cd031a9

jeremy-coleman requested a review from mweststrate March 4, 2021 13:38

jeremy-coleman added 3 commits March 4, 2021 08:48

pass the test pls

9b28d5b

remove console.log

3f7f3da

Merge branch 'main' into mobx-decorate-1.0.8

10de240

mweststrate merged commit 737cc0b into mobxjs:main Mar 25, 2021

github-actions bot mentioned this pull request Mar 25, 2021

Next release #2842

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mobx decorate 1.0.8 #2818

Mobx decorate 1.0.8 #2818

jeremy-coleman commented Feb 18, 2021

changeset-bot bot commented Feb 18, 2021 •

edited

Loading

mweststrate left a comment

jeremy-coleman commented Mar 1, 2021 •

edited

Loading

mweststrate commented Mar 2, 2021 via email

jeremy-coleman commented Mar 2, 2021

mweststrate commented Mar 3, 2021

jeremy-coleman commented Mar 3, 2021

jeremy-coleman commented Mar 4, 2021

mweststrate commented Mar 25, 2021

Mobx decorate 1.0.8 #2818

Mobx decorate 1.0.8 #2818

Conversation

jeremy-coleman commented Feb 18, 2021

changeset-bot bot commented Feb 18, 2021 • edited Loading

🦋 Changeset detected

mweststrate left a comment

Choose a reason for hiding this comment

jeremy-coleman commented Mar 1, 2021 • edited Loading

mweststrate commented Mar 2, 2021 via email

jeremy-coleman commented Mar 2, 2021

mweststrate commented Mar 3, 2021

jeremy-coleman commented Mar 3, 2021

jeremy-coleman commented Mar 4, 2021

mweststrate commented Mar 25, 2021

changeset-bot bot commented Feb 18, 2021 •

edited

Loading

jeremy-coleman commented Mar 1, 2021 •

edited

Loading