Release 0.3.17 (template updates) - ??

[joshbruce]

Description

• Fixes Update PR Template #1075

Submitter

• $ npm version has been run.
• Release notes in draft GitHub release are up to date
• Reviewer checklist is complete.
• Merge PR.
• Publish GitHub release using master with correct version number.
• $ npm publish has been run.
• Create draft GitHub release to prepare next release.

Note: If merges to master occur after submitting this PR and before running $ npm pubish you should be able to

1. pull from upstream/master into the branch holding this version,
2. run $ npm run build to regenerate the min file, and
3. commit and push the updated changes.

Reviewer

• Version in package.json has been updated (see RELEASE.md).
• The marked.min.js has been updated; or,
• release does not change library.
• case_insensitive_refs is only failing test (remove once CI is in place and all tests pass).
• All lint checks pass (remove once CI is in place).

[joshbruce]

@Feder1co5oave: Are we in the forcing merges despite failing tests phase??

[Feder1co5oave]

Currently, we are. We haven't been in an "all tests passing" status for a long time. Once we fix that test case, we'll make sure that new PRs don't introduce new failing tests.
You can easily check out what went wrong in CI by clicking the "details" link in the checks section of the PR.

[joshbruce]

Think this now covers all the feedback in #1075 except what's under the "input" header - maybe a different issue or conversation. Taking out the WIP part.

[joshbruce]

Note: Leaving in the possibility that the release doesn't actually change the library because we may need to change the README or something without actually changing the code...not sure NPM updates that without publishing a version...could be totally wrong (NPM is definitely different than Packagist).

If we can update the README displayed on NPM without publishing an update to NPM - let me know and will definitely take that part out.

[styfle]

NPM requires publishing a new version to update the README.

It should be a patch update like with

npm version patch

[joshbruce]

I suppose I could always rollback on my branch but let's see if we can do it this way.

1. In the previous sequence publishing to NPM happened before the PR; therefore, the previous Release 0.3.16 #1067 (release 0.3.16) was already published to NPM but not merged before the lint and Travis setup...my bad and hopefully not something we have to worry about moving forward.
2. Went to fix the merge conflicts on the other PR but a) really can't update anything to that version (see previous); so, no GitHub release for 0.3.16 and b) not sure but it seems GitHub or I lost each other anyway. Therefore, this branch got the update from master.
3. Added @UziTech's recommendation to automatically commit the minification step...still feels weird to me, at least on this PR because I use GitHub Desktop but I'm cool with it if everyone else is as well.
4. Accepted the recommendation from @Feder1co5oave on order of operations - starting with version and then PR before doing anything else; therefore, this little hiccup shouldn't happen again.

@Feder1co5oave, @UziTech, and @styfle: Broke and fixed some things.

[joshbruce]

[edit]: Should not have that problem again...

We also have bigger fish to fry, I think right now regarding the REDOS thing. The changes being made by this PR, beyond the actual package code, are minor and can be easily changed in future.

[Feder1co5oave]

I really thought it obvious that release PRs should only be about releasing and nothing else. Since it is not, I'm now proposing that they are, and to make this a hard rule to follow.

Releasing to npm just to update template changes that are specific to github is nonsense, because they're used only by github. We shouldn't release unless we've got a real reason to.

Is this supposed to be a draft?

[Feder1co5oave]

what's this?

[joshbruce]

This was a recommendation from @UziTech. Basically instead of build then commit min...let NPM + Git do it for us. As far as the details of what Git is actually doing I would have to read the docs.

[styfle]

We shouldn't release unless we've got a real reason to

I agree with @Feder1co5oave 💯

[joshbruce]

@styfle: I agree with @Feder1co5oave as well regarding releasing when we need to not because of some updates to documentation, which is not what caused this PR to become a release. And we have a reason. I really feel like I'm getting a little beat up over something that I was actually doing to try and help fix a problem. Not only that, but now we have a bunch of reasons to do a release.

@Feder1co5oave: Yes, that's supposed to be a draft - I suppose in my zeal and excitement of getting testing updated, the DOS solution in place, having solid docs up on the board, improving the processes and templates for us, having our first contributor who wasn't the four of us report and fix something, and so on, I accidentally hit publish instead of save draft...again, accidents happen...

Gotta step away for a minute, I think...been a busy day on here.

[joshbruce]

Actually I still see it as a draft release...with all the updates as to why it's a release. And, once we merge in #1083 - we got one more reason to do a release.

[styfle]

@joshbruce
I think the problem we are getting at is that there should be two separate PRs:

1. Update GitHub issue templates
2. Bump version and release

To quote @Feder1co5oave

release PRs should only be about releasing and nothing else. Since it is not, I'm now proposing that they are, and to make this a hard rule to follow.

[Feder1co5oave]

@joshbruce I now see it as a draft too. I think the new v0.3.17 tag confused me(?)

[joshbruce]

@styfle: Agreed. I accidentally pulled master in a way that brought the release-worthy PR submission from @Feder1co5oave into this branch because I was on it. [edit] I also accidentally merged something before we finished the 0.3.16 release on GitHub, which means we technically could not do one, because it would be out of sync with NPMJS; therefore, updated the process accordingly.

Given the overall state of things - I don't think it's the end of the world. NPM and GitHub releases are two completely different things - and neither one has actually happened yet for 0.3.17.

So, if we review the templates - without getting hung up on how we got here - approve them (which I've been using them all day today anyway).

1. We can merge in the DOS fix security: fix REDOS vulnerabilities #1083.
2. Possibly merge in the documentation fixes (to update README and all that on NPM as well, because as was pointed out by @styfle, the only way to do that is to publish a release to NPM).
3. We can then update this branch again with an updated min.

Then our potential future contributors get updated docs (including the AUTHORS file to start working on governance and community things), we get updated checklists that are cleaner, and even a security vulnerability check to boot.

@Feder1co5oave: Yeah. The tag thing confused me as well. Not sure what's going on there.

ps. Even if we don't like these templates...we can update them in pretty short order. I'm more concerned with Marked ending up on the blacklist again and us sticking so much to process that we can't handle accidents committed by contributors easily...including within the ring of committers.

[joshbruce]

We also get the improvements to the testing harness submitted by @UziTech and @davisjam. Yes, they should be two separate PRs. Yes, up to this point, they always have been. Yes, in the future, given the updated checklists, it should be harder to have something like this happen again. [edit] No, I don't think it needs to be a hard and fast rule...I don't do well with hard rules when it comes to processes; there's always an exception that will happen and the rule should not be (or, in this case, doesn't necessarily have to be) followed.