Skip to content
This repository has been archived by the owner on Jan 6, 2021. It is now read-only.

feat(vars): Convert environment vars #32

Merged
merged 2 commits into from
Oct 4, 2016
Merged

feat(vars): Convert environment vars #32

merged 2 commits into from
Oct 4, 2016

Conversation

EnzoMartin
Copy link
Contributor

@EnzoMartin EnzoMartin commented Oct 4, 2016

  • Converts environment vars used in npm run to account for windows/linux respectively
  • Add tests for command
  • Update npm test to look for all .test.js files

Essentially it makes cross-env PORT=3000 $npm_package_main work, otherwise on Windows you get Error: Cannot find module 'C:\Repo\$npm_package_main' since Windows expects env variables to be in the format of %npm_package_main%

As a side note, the linebreak-style rule makes it a frustrating experience committing against the repo as it fails all commits despite the fact that Git converts CRLF to LF automatically on commit (this is a default setting for Git on Windows)

- Converts environment vars used in `npm run` to account for windows/linux respectively
- Add tests for command
- Update `npm test` to look for all .test.js files
@kentcdodds
Copy link
Owner

Sorry about the linebreak-style issue. But without it, sometimes things can get kinda crazy. I need to add my .gitattributes file so it's more seamless. I actually need to do a few things to this project to improve it. For example, the testing stuff is sub-optimal and actually the breaking build is related to that. I'm going to make a few updates right after I merge your PR to fix things up a little. It may take a bit before I get a chance to do that though. Soon hopefully.

@codecov-io
Copy link

Current coverage is 100% (diff: 100%)

Merging #32 into master will not change coverage

@@           master   #32   diff @@
===================================
  Files           1     2     +1   
  Lines          34    49    +15   
  Methods         0     0          
  Messages        0     0          
  Branches        0     0          
===================================
+ Hits           34    49    +15   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update f2275d5...e2f7acd

@EnzoMartin
Copy link
Contributor Author

EnzoMartin commented Oct 4, 2016

I can understand about the line endings, though I've never seen it cause issues in any repo I've worked in, both personally and professionally, since Git defaults to fixing the line endings

Either way, I've fixed the lodash.assign bit to use Object.assign, should be good now

@kentcdodds kentcdodds merged commit 8da54d0 into kentcdodds:master Oct 4, 2016
@jayphelps
Copy link

FYI this broke my build

https://travis-ci.org/redux-observable/redux-observable/builds/165012081

redux-observable/redux-observable#125

> cross-env NODE_ENV=development webpack src/index.js dist/redux-observable.js
module.js:457
    throw err;
    ^
Error: Cannot find module './command'
    at Function.Module._resolveFilename (module.js:455:15)
    at Function.Module._load (module.js:403:25)
    at Module.require (module.js:483:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/home/travis/build/redux-observable/redux-observable/node_modules/cross-env/dist/index.js:13:16)
    at Module._compile (module.js:556:32)
    at Object.Module._extensions..js (module.js:565:10)
    at Module.load (module.js:473:32)
    at tryModuleLoad (module.js:432:12)
    at Function.Module._load (module.js:424:3)

@@ -1,4 +1,5 @@
import {spawn} from 'cross-spawn';
import commandConvert from './command';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't appear to be in the npm package dist artifacts
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing it's "build": "cd src && babel index.js -d ../dist && cd ..",

@triddle01
Copy link

How about bumping the version number?

jayphelps added a commit to jayphelps/cross-env that referenced this pull request Oct 4, 2016
fixes issue introduced by kentcdodds#32 using ./command.js but it not being in
artifact dist
@jayphelps
Copy link

Issue was fixed in e847b18 v3.1.1

@saumets
Copy link

saumets commented Oct 4, 2016

Ah, I just posted an issue as this broke our build just like yours did @jayphelps

Will update again and try to delete the issue if it's fixed now.

@kentcdodds
Copy link
Owner

Oh snap, @elijahmanor, I just remembered this is a thing. I don't know whether you need to use cross-var and cross-env together. cross-env can do both by itself! (Ref elijahmanor/cross-var#3)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants