-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: update npm scripts for Windows #118
Conversation
Adding https://www.npmjs.com/package/cross-env to devDependencies, and updating script invocations.
@krsanford Thanks for adding this patch to our sdk. I'm concerned that this package, cross-env has been archived and will not longer be actively maintained. Is there an alternative that is actively maintained, or is this package considered "the standard" for non-un*x based node development. Excuse my ignorance, but I haven't had to code on windows in over 15 years. |
@alexs-mparticle happy to help with my Windows machine 😄 1 - Use cross-env anywayWhile the package won't be maintained, I think that may actually help the stability of the code, as suggested by the owner:
It appears that the owner has committed to maintaining security patches if needed:
This package has been used very widely within Windows Node development and has many more dependent packages than the recommended alternatives. I think the risk presented is fairly low. 2 - Use dotenvIf you are concerned about option 1, the alternative I'd recommend would be using something like dotenv to manage the environment variables outside of the package.json scripts and inside .env files within the repository. Then, the scripts can use dotenv-cli to apply a specific set of environment variables as needed. This package is commonly used for managing complex environment variable configurations in the development lifecycle. Though, it does add more complexity by introducing a new configuration file concept and format. I would advocate this path if there were interest in more broadly applying env files as a common practice among other code repos for mParticle. Let me know what you think, and feel free to nuke this if you like! |
@krsanford Thanks for handling this. Love how well thought out your reasons are and I completely agree that it might be safe to use I personally prefer @rmi22186 and I need to talk this over and see which solution works for us long term, so we'll get back to you. |
@krsanford - We've tested on both mac and windows and tests are passing. I'll go ahead and merge this in. Thanks so much for the PR! |
🎉 This PR is included in version 2.12.9 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Adding https://www.npmjs.com/package/cross-env to devDependencies, and updating script invocations.
Summary
This fix addresses open issue #88 #88 . Currently, npm scripts fail when run on a windows environment. Implementing cross-env as a dev dependency as suggested in the issue.
Testing Plan
I first verified that I was able to recreate the issue as presented in the issue. After implementing cross-env, I tested locally on a Windows 10 machine, running all updated npm scripts/paths successfully. npm run test and npm run build complete to success as executed from a Powershell console. This will require further testing on other Dev OS's.
Master Issue
It looks like https://go.mparticle.com/work/ is behind an mParticle auth wall. I'm not sure what Master issue this closes, but it relates to issue #88