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

Rewrite Makefile using NPS Scripts; Closes #2352 #3194

Closed
wants to merge 1 commit into from

Conversation

TedYav
Copy link
Contributor

@TedYav TedYav commented Jan 7, 2018

Addresses #2352

Description of the Change

Where possible, I tried to exactly duplicate the design of the Makefile using nps. I've tested it by running build and test commands, as well as granular testing of individual commands against a vanilla repo to ensure it's running the same commands.

Important notes:

  • removed the ==> [<ACTION> :: <SUBACTION>] output from makefile as nps does this automatically
  • did not recreate make .PHONY as this appeared to be a hack for GNU make. I'm not sure if some kind of file watching capability is desired. Perhaps this can be added back if needed.
  • removed the dependency check (SRC_FILES variable) from Makefile as I didn't see a big reason to include this in nps build command
  • code coverage can now be enabled by:
    • setting environment variable COVERAGE to any value
    • passing "--coverage" in command line to nps (though nps makes this inconvenient)
    • adding the withCoverage directive to the nps task list, i.e. nps test.node.bdd withCoverage
  • make is now nps build
  • any scripts which depend on make but were not in package.json will be broken
  • nps will need to be installed globally to work well, or at least be included in user's $PATH
  • npm run <xxx> commands are now npm start <xxx> or nps <xxx>

Alternate Designs

Possible modifications:

  • Can use npsUtils.concurrently.nps to run tests in parallel. This has pros and cons.
  • Can add documentation on how to use nps if needed
  • I'm tempted to eliminate all the constants from the top for NYC, MOCHA, etc, though I imagine these may be useful in case a developer wants to temporarily direct these scripts to use a different installation

Why should this be in core?

Discussed this briefly with @boneskull on Gitter back in early 2016. Didn't get around to doing it till now (my bad).

Benefits

Makefile gone!

Possible Drawbacks

Any code that depends on make commands will need to be updated.

Applicable issues

This should not be a breaking change, unless there is some code I'm not aware of that depends on make commands (possibly some CI type operations)

@jsf-clabot
Copy link

jsf-clabot commented Jan 7, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Ted Yavuzkurt seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

Copy link
Member

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Thanks. .travis.yml and appveyor.yml need to be updated as well, since the builds are now failing... not sure about Netlify. @Munter?

@boneskull
Copy link
Member

@TedYav Looks like you'll need to sign or re-sign the CLA; maybe it couldn't link your account?

@boneskull boneskull added type: chore generally involving deps, tooling, configuration, etc. semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Jan 7, 2018
@TedYav
Copy link
Contributor Author

TedYav commented Jan 8, 2018

@boneskull ahh I pushed this from my work laptop and I think I had different email in my git settings, will resubmit with proper configuration. Also will update CI configurations

@TedYav TedYav closed this Jan 8, 2018
@TedYav TedYav deleted the make-to-npm-scripts branch January 8, 2018 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes" type: chore generally involving deps, tooling, configuration, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants