search

jaredpalmer / tsdx

Zero-config CLI for TypeScript package development
https://tsdx.io
MIT License
11.27k stars 507 forks source link

Improve template CI: add matrix testing, use `bahmutov/npm-install` #882

Closed agilgur5 closed 4 years ago

agilgur5 commented 4 years ago

Description

Commits

fix: use @bahmutov/npm-install in templates' CI

feat: add test matrix to all templates' CI …

ci: make internal job names more consistent w/ templates' …

Tags

References

Fixes

This fixes #799 because bahmutov/npm-install isn't susceptible to that issue. Would still like to hear from upstream why they used **/yarn.lock but https://github.com/actions/cache/issues/400 hasn't gotten a response in over a month 😕

vercel[bot] commented 4 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/formium/tsdx/f111ug5jz
✅ Preview: https://tsdx-git-fork-agilgur5-improve-template-ci.formium.vercel.app

jaredpalmer commented 4 years ago

While I appreciate the idea of this, I am not a fan of this PR. Spinning up this large of a matrix is unnecessary for a significant percentage of tsdx users. I can understand perhaps testing on 3 version of node, but not on also against windows and macos as a default. The latter should just be a recipe in the docs and not the default for every package.

agilgur5 commented 4 years ago

Why is it "unnecessary"? Who is "a significant percentage"?

Matrix testing on different platforms and OSes is almost always good to have to ensure compatibility, and the limitation is usually set-up effort (which this removes) or CI minutes (for OSS it's free, otherwise can always remove this section).

As written above, this was proposed in #438 where you didn't have any opposition. This is not a new idea and not my proposal. As I mentioned there too, anything in the templates is much easier to remove than it is to add. I'd rather add most best practices out-of-the-box and let users delete these if they so choose, than make them opt-in and require effort by users to have best practices. The more effort it requires, the less likely it is to happen, and when it comes to best practices, that means less awareness, less compatibility, less safety, etc

jaredpalmer commented 4 years ago

You released 14 (which had breaking changes) without giving me notice and without a single a prerelease. I don’t have time to review every PR on every project of mine, but I read all release notes religiously. Had this (and size-limit) just been in a prerelease I would have suggested changes prior to releasing to everyone, so the argument that “I didn’t see it” is mute.

Anyways, there is a cost to the matrix as it stands now. It slows down PRs and increases action minutes (which cost money on private repositories). I don’t see a big reason why we are even testing in multiple versions of node out of the box. Node 12 is sufficent. These are all things that can be customized and are heavily project dependent. All that’s necessary is a bit of documentation.

agilgur5 commented 4 years ago

This is another comment that was very hypocritical and disrespectful. I literally didn't respond to this for weeks and this ruined a day or several for me. I'll ask again that you 1) read the full context before responding and 2) please consider how your words and actions have and continue to significantly damage my mental health and what that means for me and the community.

which had breaking changes

Neither this PR that has matrix testing nor the size-limit PR #705 are breaking... In fact, #705, this, and some others were originally planned for v0.13.x, which is literally called "Template Improvements".

They are also not listed as breaking in the v0.14.0 release... The few changes that are listed as breaking are called "Slightly Breaking", are only changes in dependencies, and I anticipate the vast majority of users (if not all) will have no breakage during upgrade.

without giving me notice

I wrote in March, literally immediately after getting publishing rights that I'd be using milestones. Until recently, there were only 3 milestones, v0.13.x, v0.14.0, and v0.15.0, each was created either in March or early April, and I don't believe I've edited their descriptions or names since then. I also told you in June repeatedly that there's a very transparent and consistent process to everything, I use milestones, I heavily make use of labels, use RFCs, rotate pinned issues, sort issues with vote tallies, use prefixes, re-write titles, write very descriptive commits and PRs, and that I don't push to master, ever. (and would like to be even more transparent if GitHub's Projects had more project management and planning features such as Epics, or issues in multiple milestones even)

438 was proposed in January by Shawn, and you literally responded to it yourself with no objections... #705 was in May, and something similar existed in the codebase prior to May 2019, you literally asked for something similar in February, and then again in August, which I responded to and linked to what it duplicated. You did not respond back.

I didn't say you "didn't see it", you literally did see them and either requested or approved of them...

I also literally wrote in the v0.13.3 release that v0.14.0 would be next and literally linked to the milestone there...

And on top of that, after I've merged dozens of PRs, you're requesting to undo mine and contributors' work that you didn't bother to review or respond to for months... That's not how planning works and is frankly disrespectful to the time contributors and reviewers have already put in. The corporate analogy here is "someone from up top jumping in and asking a team to change a variety of things despite not being in project discussions for many months and despite previously approving for and asking for said things".

without a single a prerelease [sic]

Can you explain why this deserves a pre-release? This was planned for many months transparently, is well tested (I've made tsdx test dogfood itself, am planning on having tsdx build dogfood itself, have contributed to various upstream and upstream upstream repos, and have written, requested, and coached in more tests than anyone and would've written more if not for things like https://github.com/formium/tsdx/pull/407), was blocking progress on many other things, was causing lots of user issues, and all of the breaking changes are necessary. There's nothing experimental in them. If I have to make a pre-release solely for you to read, that sounds like a problem. I've already told you repeatedly that I'm not making exceptions for you, especially not when I don't even make exceptions for myself. That's a bad role model and sets bad precedent. I strive to treat everyone equally and be transparent to everyone, planning in the open. And as I've written twice already, they're also only "Slightly Breaking" and won't even impact the vast majority of users. And the changes you have a problem with are, again, not breaking and were originally slated for v0.13.x.

This is also extremely hypocritical. You've made 0 prereleases for TSDX, even though some of these releases were extremely breaking, and some patches were breaking and I've had to point out and fix broken releases before. Users expressed displeasure with those as well. This comes off as "do as I say, but not as I do".

I've also put in lots of stability work as I've written and intentionally made various efforts to make things backward-compatible where feasible, even adding the only deprecation notices that exist in TSDX. You're hypocritically barking up the wrong tree here. I'll make prereleases for things that are experimental and need extra testing, but I'm not making prereleases just so you can read and give feedback to PRs that were made months prior and have gone through multiple iterations before being merged.

the argument that “I didn’t see it” is mute.

I didn't even say that, but again, you did in fact see them.

But frankly, that's an absurd statement. It's one thing to give feedback, but it's a totally other thing to blame me for the fact that you abdicated responsibility, you don't read any issues, PRs, milestones, releases, or emails, you don't respond to anything, and you didn't ask for anything. That's abusive and a hypocritical double-standard.

Instead of thanking me for being the only maintainer for close to a year -- dedicating ~1000 hours to this and even ignoring some of my other repos, responding to all issues, debugging people's code, reviewing all PRs through multiple iterations and contributing to many of them, organizing, labeling, and deduplicating hundreds, adding all contributors, including issues, PRs, and contributors from well before I was a maintainer, adding lots of labels, adding milestones, being transparent, adding lots of tests, contributing upstream and upstream of upstream, striving to make things more backward-compatible, fixing lots of bugs, adding major features, writing 100+ PRs -- all without anyone asking me or telling me anything (zero direction) -- the few times you jump in you're seemingly constantly telling me that I'm the problem and that I'm not doing enough, even when you didn't do those things yourself...

agilgur5 commented 4 years ago

Anyways, there is a cost to the matrix as it stands now. It slows down PRs and increases action minutes (which cost money on private repositories).

Here is my previous comment:

or CI minutes (for OSS it's free, otherwise can always remove this section).

The increased time of CI on the matrix is negligible for most projects. Really large, performance intensive projects generally require a lot of tuning, especially in CI, so I think that's par for the course there. As I wrote, I'm talking default, best-practices for open-source repos.

I don’t see a big reason why we are even testing in multiple versions of node out of the box. Node 12 is sufficent.

Here is my previous comments:

Matrix testing on different platforms and OSes is almost always good to have to ensure compatibility, and the limitation is usually set-up effort (which this removes) As I mentioned [in #438,] I'd rather add most best practices out-of-the-box and let users delete these if they so choose, than make them opt-in and require effort by users to have best practices [...]

I think if your primary target is browsers, perhaps you don't need to test on all versions of Node, you'd want to test on multiple browsers instead. Though ensuring install works too is good. And React projects can be agnostic to platform, like Ink is React for CLIs. I'm open to changing that for the React templates, but the basic one may often be used for Node libraries.

In either case, as I've written elsewhere, the templates should really be treated as starting points and not ending points.

Node 12 is sufficent. [sic]

Why is it sufficient? Node 10 is not EOL and I think if you're only going to test any version, it should be the minimum compatible one (also mentioned in #733, which similarly refers to #438). It's generally good practice to maintain support for EOL versions for some time too to allow for more gradual migrations if it is not overly difficult to do so.

You haven't answered my questions and are stating opinions matter-of-factly without pointing to supporting evidence or backing rationale. That doesn't make for a productive discussion and makes me feel like my opinion doesn't matter.

These are all things that can be customized and are heavily project dependent. All that’s necessary is a bit of documentation.

As I've said, I'm focusing on best practices for open-source repos, that wasn't my proposal (but I agree), and it's significantly easier to remove config then add it. People have in fact been appreciative of the CI config as it saves them time. As I wrote previously:

The more effort it requires, the less likely it is to happen, and when it comes to best practices, that means less awareness, less compatibility, less safety, etc

TSDX itself is all about less config and best practices out-of-the-box, and matrix testing aligns with that.