Skip to content
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

Merged
merged 1 commit into from Apr 8, 2021
Merged

Conversation

krsanford
Copy link

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

Adding https://www.npmjs.com/package/cross-env to devDependencies, and updating script invocations.
@alexs-mparticle
Copy link
Collaborator

@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.

@krsanford
Copy link
Author

@alexs-mparticle happy to help with my Windows machine 😄
Yeah, I was disappointed to see that cross-env has been archived. I have a few thoughts on options:

1 - Use cross-env anyway

While the package won't be maintained, I think that may actually help the stability of the code, as suggested by the owner:

However, because of the extra features which were added (features which I never needed and only sometimes understood), the codebase has become a hodge-podge of different people's attempts at making things work.

It appears that the owner has committed to maintaining security patches if needed:

No new features will be added (and old features will not likely be removed). I don't plan to do any more than fix security/critical bugs and keep the Node.js support up-to-date.

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 dotenv

If 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!

@alexs-mparticle
Copy link
Collaborator

@krsanford Thanks for handling this. Love how well thought out your reasons are and I completely agree that it might be safe to use cross-env even though it's technically archived.

I personally prefer dotenv since I use that exclusively for backend JS projects, though I do agree with you that it adds extra complexity to our build process, so we'd have to weigh the options.

@rmi22186 and I need to talk this over and see which solution works for us long term, so we'll get back to you.

@rmi22186
Copy link
Member

rmi22186 commented Apr 8, 2021

@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!

@lgolan
Copy link

lgolan commented Jun 23, 2021

🎉 This PR is included in version 2.12.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

4 participants