-
Notifications
You must be signed in to change notification settings - Fork 84
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
2.0.0-alpha.2 #88
2.0.0-alpha.2 #88
Conversation
The default timeout for one test was too low in case the test was run from a slow machine. This increases the timeout to 4 seconds.
unreleased | ||
========== | ||
|
||
* deps: mocha@7.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.
Traditionally the history file is what the end consumers will see. The version of mocha we use internally doesn't change anything for the end user.
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.
Ok, I just added all of them. I will remove all of the dev dep changes (which is most of 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.
Cool, no problem. The dev dep commits are typically pre-fixed with build:
since they are part of the "build sub system" (from Node.js terms), and then by consequence all build stuff is just left out from the history file.
I tend to think of the history file's target audience is someone who uses this module directly and wants to know, at a quick glance, what the general list of changes that would matter to them when going from version X to Y.
@@ -382,6 +382,64 @@ router.route('/pet/:id') | |||
server.listen(8080) | |||
``` | |||
|
|||
## Migrating to 2.x from 1.x | |||
|
|||
The main change is the update to `path-to-regexp@2.0.0`, which has a few breaking changes: |
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.
The version listed here us different from what is in the history file?
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, yep must have missed that in the original re-update. I will take a quick read through that whole commit while I rebase to make sure I didn't miss it anywhere else.
@@ -38,6 +38,7 @@ | |||
}, | |||
"scripts": { | |||
"lint": "eslint --plugin markdown --ext js,md .", | |||
"lint-fix": "eslint --plugin markdown --ext js,md . --fix", |
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 isn't necessary; just run npm run lint -- --fix
@@ -0,0 +1 @@ | |||
package-lock=false |
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.
Is this necessary? It is already git ignored. With this set, it would mean that locally if we see a vulun, running npm audit
to see what it is won't work, since there won't be a lock file.
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.
Unless you blow it away regularly locally, ignoring it only means your lock will be in a different state than mine and anyone else who works on the package. IMO it is better to run npm i --package-lock-only && npm audit && rm package-lock.json
when you want the audit than risk the out of sync behavior on accident.
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.
Sure, but doesn't that argument apply to node_modules
already? What makes sure both of ours stays in sync?
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 guess you can make that argument. I guess it is just a more common workflow to clear out node_modules
and if you forget the lock, which is very commonly committed to to projects, you will succeed in never getting the new updates. Much more error prone IMO.
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 guess, but on that argument, then this module should also assure that npm-shrinkwrap.json
or the yarn.lock
files are also not created? It sounds like your argument is that the user will half-know their tooling (i.e. they are aware their tooling creates a node_modules
folder, but not that it is making a package-lock.json
file) so we should at least try to help out for that use-case. Unless I'm misunderstanding the use-case for this.
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.
That is probably also a good idea, although I don't use yarn and do not know a way to fully disable, and the docs also don't mention it. As for a shrinkwrap, it is much less commonly used and not created by default, it is much less of an issue.
The problem with package-lock.json
is that it is created by default without that setting on every install, and you have to be very vigilant to remove it before testing to ensure you wont just break in CI.
@dougwilson I would like to get this published this weekend. I will update the changelog/readme/package.json like we talked about, but can you give me publish rights on npm so that when I set down to do it I can do it all at once? |
I woud like to review things first. I mean, I reviewed it 17 days ago and then there were no changes, so not sure what the sudden rush is... |
Sorry, I didn't mean to make if feel like a sudden rush. I didn't make the changes because my schedule is such that I can only commit multi hour blocks once in a while, so I was hoping to queue up this and a few other things for me to work on this week(end). I just didn't want to make the change then wait another 2 weeks before landing :) |
So we are both here :) even if you made the changes say this wekeend, and they were good, I could publish after you finished your time. Also, I can always help by making changes in your PR if you invite me to do so; I just wouldn't by default because not sure if that would feel bad or even simply end up with conflicts due to maybe you were doing it offline. On a lot of these things feel free to invite me to make a change if you agree to it and I'd be happy to help out in that way 👍 |
Part of the point is to distribute the publish rights across the maintainers. This has many effects, but the main one being feeling like we can be agents of progress. IMO, you being the sole publisher is a blocker to growth, I was hoping this would be a nudge in the right direction. Once I am comfortable publishing these modules (and you are comfortable with me doing so), I can then teach others. Without this first step the project will never see the contributor growth we were hoping for. |
There is a lot to discuss there and I would rather discuss it somewhere not in a random PR for visibility and since it is off topic. Perhaps we should schedule another Express TC meeting for this coming Wednesday (next week, not today due to short notice)? |
Sounds like a great idea! Next week Wednesday I am out for a work event, but Monday, Tuesday or Friday also work for me. I could also to today, but agree that would be REALLY short notice lol. |
I could also do today, but just didn't feel it would be fair to other members who may want to join live. If we are going to change the weekday it is on, do we need to get a doodle rolling to check with the rest for which day works? |
Maybe we can make an issue and ping everyone? If it works out then great, if not we look for a time next week. |
Yea, let's do that. I know not everyone will respond, but I would hope at least the ones that attended the last would yay/nay on it, haha. I will be out of a meeting at work soon and can spin up the meeting invite, but you're welcome to do it before I get to it 👍 |
Hey, I didn't see an issue and was busy until now. It might be too late to get it done today but when I get home tonight I will post something and see if we can pick a time between now and next Tuesday. I am fine even if it is over the weekend if that works for folks. |
@wesleytodd in the interest in helping, since there have been some unreleased changes on |
All of the changes here have now been picked out into |
Prep for next alpha release. Updated dev deps and merged other dep update PRs. We should be able to also merge #86 to this hopefully.