This repository has been archived by the owner. It is now read-only.

Cross-platform scripts. Fixes #305 #350

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@benoror
Copy link
Member

benoror commented Jun 12, 2015

Let see if Travis pass. Can someone on OS X test it as well? Also another win32 wouldn't be bad!

If it works I can extend it for all scripts.

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jun 12, 2015

Looks awesome. If the tests pass, I'll pull it down, try it out and let you know. If that works, then you can update the PR to do this for all the scripts. That's awesome!

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jun 12, 2015

Ah! We're really close. It totally works, but it's a bit annoying. The output looks like this:

npm test

screen shot 2015-06-12 at 1 28 12 pm

npm run build

screen shot 2015-06-12 at 1 29 03 pm

When it should look like this:

npm test

screen shot 2015-06-12 at 1 29 49 pm

npm run build

screen shot 2015-06-12 at 1 30 26 pm

Also, if we could make a more generic function that would be great. There's a lot of duplicate code in those two script files.

@benoror

This comment has been minimized.

Copy link
Member Author

benoror commented Jun 14, 2015

The output looks like that because stdout is streamed directly to console.log.

Maybe directly using process.stdout.write can fix it.

I'll try again tomorrow ASAP, as I will optimize the code for all scripts :)

@benoror benoror force-pushed the benoror:iss305-2 branch 2 times, most recently from 139951f to 4325f69 Jun 15, 2015

@benoror

This comment has been minimized.

Copy link
Member Author

benoror commented Jun 15, 2015

I fixed it accordingly, but the introduction of --config $npm_package_webpack breaks the script, how should be handled?


switch(process.argv[2]) {
case 'build':
command_line = 'webpack --config $npm_package_webpack --progress --colors';

This comment has been minimized.

@kentcdodds

kentcdodds Jun 15, 2015

Member

Yeah, I wondered about this one... Just put './node_modules/kcd-common-tools/shared/webpack.config.js' in directly instead of $npm_package_webpack

@benoror benoror force-pushed the benoror:iss305-2 branch from 4325f69 to b039b1b Jun 15, 2015

@benoror

This comment has been minimized.

Copy link
Member Author

benoror commented Jun 15, 2015

Ok, let me know if this works for you, so I can extend it for most scripts 😄

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jun 15, 2015

This works awesome! I have an idea though... One sec...

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jun 15, 2015

Why don't we create a library for this? I want to use this in other projects. Here's the API I envision...

I install your library as a dependency. We'll call it better-npm-run for now... Then in my package.json, I do this:

{
  "scripts": {
    "build:dist": "better-npm-run",
    "test": "better-npm-run"
  },
  "betterScripts": {
    "build:dist": "NODE_ENV=development webpack --config $npm_package_webpack --progress --colors",
    "test": "NODE_ENV=test karma start"
  }
}

Or something like that? It would just be nice to have all my scripts right in the package.json.

And the library would manage ensuring that setting environment variables work cross-platform. What do you think?

@benoror

This comment has been minimized.

Copy link
Member Author

benoror commented Jun 15, 2015

Sounds good! Let me create a repo and a beta version of it 👍

@benoror

This comment has been minimized.

Copy link
Member Author

benoror commented Jun 15, 2015

Would this kind arrangement look ok? or is it too verbose?

{
  "scripts": {
    "build:dist": "better-npm-run",
    "test": "better-npm-run"
  },
  "betterScripts": {
    "build:dist": {
        "command": "webpack --config $npm_package_webpack --progress --colors",
        "env": {
            "NODE_ENV": "development"
        }
    },
    "build:prod": {
        "command": "webpack --config $npm_package_webpack --progress --colors",
        "env": {
            "NODE_ENV": "production"
        }
    },
    "build": [
        {
            "run": "build:dist"
        },
        {
            "run": "build:prod"
        }
    ],
    //...
    "test": {
        "command": "karma start",
        "env": {
            "NODE_ENV": "test"
        }
    }
  }
}

@benoror benoror referenced this pull request Jun 15, 2015

Closed

Initial brainstorming #1

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jun 15, 2015

Hmmm... That is a bit verbose, and it prevents me from doing this: NODE_ENV=something some-command && NODE_ENV=somethingElse some-other-command though there's a work-around... Hmmm... Probably would be fine to start with what you have 👍

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jun 15, 2015

Sorry, by the way, I just fixed up a thing or two with package.json so that's why this can't merge anymore :-(

@benoror

This comment has been minimized.

Copy link
Member Author

benoror commented Jun 15, 2015

Okey, already created a draft version at github.com/benoror/better-npm-run.

Right now I don't have any project that could use it, but if you think it has potential to be used by angular-formly or anything-else in the future let me know.

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jun 15, 2015

Awesome! I'll take a look at it!

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jun 16, 2015

Hey man, just tried it out on my new library and it worked great for me. I didn't even notice a difference. 👍

@benoror

This comment has been minimized.

Copy link
Member Author

benoror commented Jun 16, 2015

Hey that's great @kentcdodds! 😄 considering that I wrote the draft on-the-go this afternoon.

Anyway, please let me know if you have a particular use-case or idea for extending its functionality, I opened a general issue for brainstorming: benoror/better-npm-run#1

Regards!

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jun 16, 2015

Thanks for building that! I've implemented it. Please test it out :-) 6fbaea2

@kentcdodds kentcdodds closed this Jun 16, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.