-
-
Notifications
You must be signed in to change notification settings - Fork 230
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
feat: migrating utility modules to monorepo #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good! Loving the reduction of duplication. And the README is great, nothing to change there other than adding a ## Quick Start
for contributing / getting tests running etc.
package.json
Outdated
@@ -1,5 +1,32 @@ | |||
{ | |||
"name": "istanbuljs", | |||
"version": "1.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're calling this the supporting libraries for the IstanbulJS 2.0 CLI, so shouldn't this start at 2.0.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JaKXz slightly confusing I know, but we've been using 2.x
to refer to the process of splitting Istanbul into a series of packages rather than one monolith -- I don't disagree with you, but I think it might not be worth a major bump since several libraries rely on these modules already and it would be confusing to them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this is just an internal number right? Are external deps requiring istanbuljs
right now? Unless I'm misunderstanding a lerna concept right now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, shoot! you are 100% right :p I was thinking you meant the packages within the repo -- because I'm bad at reading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha no worries. One more question though, will this increment in minor versions [only] for any changes to sub-packages, except breaking changes? That would make sense to me... though I'm not entirely sure.
Or do we want to go with the babel versioning system and start at [an arbitrary version i.e.] v10 [i.e. like nyc] and increment everything together?
"scripts": { | ||
"test": "cross-env LERNA_TEST_RUN=1 nyc lerna exec npm test", | ||
"postinstall": "lerna bootstrap", | ||
"release": "lerna publish --conventional-commits" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
"test": "istanbul cover -x 'docs/**' --include-all-sources --print=both _mocha -- test/", | ||
"posttest": "istanbul check-coverage --statements 95 --branches 80", | ||
"release": "standard-version" | ||
"test": "_mocha -- test/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra space here
@@ -7,8 +7,7 @@ | |||
"scripts": { | |||
"fast": "mocha test/", | |||
"pretest": "jshint index.js lib/ test/", | |||
"test": "istanbul cover -x 'docs/**' --include-all-sources --print=both _mocha -- test/", | |||
"xposttest": "istanbul check-coverage --statements 95 --branches 80" | |||
"test": "_mocha -- test/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another nit
"test": "istanbul cover --include-all-sources --print=both _mocha -- test/", | ||
"xposttest": "istanbul check-coverage --statements 95 --branches 80", | ||
"release": "standard-version" | ||
"test": "_mocha -- test/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another nit.
Question, though, why do these use the _mocha
executable while istanbul-reports
doesn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is an Istanbul thing, we could switch to mocha
; it might be worthwhile to do so.
_mocha
doesn't spawn a subprocess mocha
does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vote for switching, since that will be more consistent with 'correct' usage patterns... and would serve as a pseudo acceptance test...
.travis.yml
Outdated
node_js: | ||
- '4' | ||
- '6' | ||
- '7' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend using the 'stable'
keyword instead of specifying non-LTS versions so it's one less thing to maintain as time goes on.
@graingert nope :) I see this as the first pull request on top of the unification pull-request 👍 Also, cool thing worth noting as we pull in a few more libraries., there's a |
Improved team city reporting
this moves us closer to wrapping up the work on migrating our utility modules into a monorepo:
I'm convinced that moving to a monorepo will help better centralize bug tracking, and will keep conversations centered around the more interesting modules in the istanbul ecosystem.
Not a blocker ... but ...
I would love some help debugging why the
istanbul-api
tests have trouble running when instrumented withnyc
.