-
Notifications
You must be signed in to change notification settings - Fork 66
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
Typescript, Rollup, Jest, Terser, Rimraf, announcer, de-binarying, CI platforming #107
Conversation
I'll do a nice HTML coverage report later, I'm sleepy |
I like the "lcov" reporter from nyc. It will generate both HTML and lcov. The tests we currently run against the compiled version are by hand in browsers; I was planning on using playwright to automate that (at least for the most modern browsers). |
I think that's a fantastic choice Fairly warned: merging Does the general direction of this PR seem suitable so far? |
I've never checked in coverage output before. Can you talk a little bit about why that's interesting? |
we either need to drop the output in |
this is probably the right choice.
this is probably also the right choice. i think the umd should go where the old one was, and we should add a |
When you're ready for this to be reviewed, can you please rebase onto main and squash to one or more commits that have your train of thought shown, then tag me for a review? |
Rebase: no problem Squash: actually none of my jobs have ever permitted that. I imagine it won't be a problem? 😅 but in a decade of using git I've never actually learned how |
heh, i had to learn about this on #89. See https://stackoverflow.com/questions/6217156/break-a-previous-commit-into-multiple-commits/6217314#6217314 for a pretty good writeup. Look for the instructions for |
And don't worry that you have to force-push to your branch; GitHub will do the right thing with the PR. |
this is going to make me do 40 manual merges do i have to squash? this is done but that'll take hours and is defect prone |
You can see an effective squash here |
What I did was squash down to one commit, then break it apart by file into changes that could layer onto one another. There was only one file that was changed in two different ways, and it wasn't hard to break that file apart. If it's really too much work, I'll take a look at it myself over the next few days. |
Please understand that when you say this, you are creating high costs for me without taking the time to understand first.
To take this out and re-do it the way you suggest is effectively starting over and tripling the workload, and introducing the opportunity for several layers of bugs. The benefit appears to be that you don't have to look at a I respectfully decline.
You're going to ask me to scrub this PR, and make an entire new PR, for a single line? I respectfully decline.
Mocha won't work with rollup at all in any practical typescript context, because it's ancient. What this means is either I have to replace the packaging and the compiler with no tests, or I have to do what I already did. This is one line and one configuration file. This doesn't need a separate PR.
This is you requesting an entire PR for a single flag on a thing you want me to do in a different PR. No, we don't need two PRs for Asking for changes like this is very frustrating. These were in the same PR for a reason. I don't generally do that; my PRs are generally small; I've repeatedly expressed that this PR is too large.
If these didn't need to be colocated, they wouldn't be. It would be polite to ask why they are, or try to figure it out, before requesting changes.
The purpose of the Github Actions workflow is to increase the quality of testing. I'm sorry, but no, the quality of testing needs to be increased before core build processes are altered. Because the GHA is reliant on changes in this, either we put the GHA in its own PR then change it in this anyway, or we just do it the way it's already done.
Respectfully, the kind of feedback I was hoping for was errors and omissions. |
I would like to fix the linting errors, but, to re-introduce linting to the build pass after landing this. The reasoning is ugly.
Would this be acceptable? I just don't want this PR to grow to cover more ground, but it has to to get these things in the right order, unless we just ignore the automation for a few hours |
This will not impede the CI lockstep for #1 , which is handled in a different branch |
are node users going to use the code that's been through rollup after we release this way? |
Node users will use code that has been through If we decide to, we can also have one missing the last step on CDN. That's good for people when they're debugging. I haven't done that yet, and it should only be on CDN, not in the module |
At this time, that includes |
This is considered first-pass review responded. Outstanding problems that I hope to kick the can down the road for into followup PRs:
|
for some reason it won't let me re-request review from @Mingun. this is not intended as a snub: the button just doesn't work (worked fine for hildjj, not sure what the corned beef is) |
I think I'll have time to do the final review on this today. I don't think we need anymore back-and forth until I'm done with that. |
Creating a good PR is not a simple task, yes. I don't think we should rush to merge unpolished PRs. Clear history has many benefits, for the maintainer in the first place. Polishing, I think this is quite normal practice, which shows respect for all participants in the process for each other. David set a very high bar for the quality of the code and commits in the repository, I would not want to lower it.
I suggest only general pipeline. You feel free to merge or reorder some steps if you feel that this is necessary. It even doesn't needed to build correctly on all targets if you can't accomplish this in one commit (but, of course, this would be desirable). Just give a clear explanation in the message why this can't be done right now, and fix that in the next commit or two.
Even in that case you can leave only two commits, instead of dozens:
No (although this is the point... make changes in small portions). Just that is a pretty isolated thing and that can be done very early and keep the history as clear as possible. I think, it is not depend on the rollup/jest/other changes, right?
Well, it's good that I suggested replacing it before switching to rollup 😄
In the end, you do not touch tests at all, so the change of the testing library was smoothly, isn't it?
It seems to me that you are overreacting emotionally and see in the comments what was not there. I do not suggest doing other PRs, I suggest doing rebase this. My apologies.
Agree, in that current state. Although final changes are small enough
Hmmm? I said the same thing, in my suggestion, changing the pipeline comes first! |
None of what you requested would have led to a good PR, is the problem, and if you'd do your new work yourself, you'd have sufficient experience to understand the problems you're creating, while asking the labor from someone else.
There's no lack of polish here. You misrepresent your preferences as quality. Of the entire stack here, the only problems have specific technical reasons for them.
No, I cannot, and I already explained why. I have already concretely told you no several times.
No, it isn't. If and when you try to do this work yourself, you may learn why. I cannot explain a fourth way. I'm sorry none of the existing explanations reached you.
Well, good luck doing it yourself. You don't seem to understand what's wrong here.
Nope. Sorry you don't understand why. You'll learn when you try to do this work yourself, without me, or end up stuck in tools that are more than a decade old.
That's nice.
You've been told |
You guys can have this code. Do with it as you please. It was written MIT. Use it. I make one recommendation: some of the changes you want to make aren't as easy as they sound. Probably make a new branch and work in that, so that if you change your mind, you can change your mind. Land this directly if you want. |
This went in, largely unmodified, because it's high quality work and you need it. |
Do not merge
Please hold, this removes the header process accidentally
This will resolve:
Behavioral changes
This now runs all steps as part of a build - previously, testing and static analysis was a separate call. If you want just the build, now use
npm run make
.This is a strictly test-additive change, and should cause no breakage
Want to try it?
npm run build
Fixes #100, fixes #96, fixes #94, fixes #90, fixes #83, fixes #77, fixes #75