-
Notifications
You must be signed in to change notification settings - Fork 470
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
Build: replace grunt tasks with npm scripts #512
Conversation
12d03f5
to
84b4216
Compare
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.
A few comments.
Includes: - **lint**: lint all code using new eslint flat config - **build**: build using rollup, minify using uglify, process for dist - **npmcopy**: copy some node module files to the external folder - **start**: watch source files and rebuild on changes using rollup.watch Also: - Update the build function in the release script to use the new build - Update CONTRIBUTING.md and README.md with new scripts - Confirm the min file was identical to the main branch build, aside from the version update. - Keep uglify-js at 3.9.4, which is the last version that officially supported the ie8 argument, which we need for ie9 Closes jquerygh-512
Includes: - **lint**: lint all code using new eslint flat config - **build**: build using rollup, minify using uglify, process for dist - **npmcopy**: copy some node module files to the external folder - **start**: watch source files and rebuild on changes using rollup.watch Also: - Update the build function in the release script to use the new build - Update CONTRIBUTING.md and README.md with new scripts - Confirm the min file was identical to the main branch build, aside from the version update. - Keep uglify-js at 3.9.4, which is the last version that officially supported the ie8 argument, which we need for ie9 - update browserstack project name to jquery-migrate - run browserstack at least once a week Closes jquerygh-512
src/jquery/core.js
Outdated
migratePatchFunc( | ||
jQuery.fn, | ||
"init", | ||
function( arg1 ) { |
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'd like to avoid all this reformatting in this file. We don't use Prettier and any time we do major code style changes like that - especially ones modifying line numbers - it gets harder to look at history.
If something is a result of stricter ESLint then fine, but this looks excessive to me.
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 don't really know of a better way to format this. I understand the concern about diffing, but I think the way Prettier formatted this is the way it should have been formatted from the beginning. And, doing it now makes it less likely we'll have to make more formatting changes in the future. It was ESLint that prompted this, but I prefer to let tools do the formatting as much as possible so that maintainers like us don't waste any time doing delicate dances to get the code to pass lint.
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.
Well, there is also the current style. Right now, this PR splits the style as other files use what was here before; you'd have to reformat everything to make it consistent again.
What exactly in ESLint triggered these changes?
I prefer to let tools do the formatting as much as possible so that maintainers like us don't waste any time doing delicate dances to get the code to pass lint
I have a more nuanced view on this. I do use formatters a lot - and usually it's Prettier as well - as it helps with avoiding some discussions, especially in large teams. But integrating such a tool doesn't mean you stop having formatting changes, unfortunately. Prettier's GitHub has many discussions about how to best format various code constructs and they change things from time to time. In my experience, having Prettier in a project means that you periodically need to update it & then reformat a huge percentage of files, creating huge diffs and obstructing the Git history. In larger teams & projects I believe it's still worth it but I have doubts about jQuery with just a few regular committers and quite rare source changes. In some files, we'd end up with the only modifications being occasional Prettier runs.
One option is to employ usage of https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view, although I expect it's not perfect and won't erase all the noise.
As for the current Migrate style as it's on main
, I actually like it as the following:
migratePatchFunc( jQuery.fn, "init", function( arg1 ) {
/* code */
}, "selector-empty-id" );
is quite similar visually to:
jQuery.fn.init = function( arg1 ) {
/* code */
};
which is mostly what happens, modulo the migratePatchFunc
wrapper that is needed for being able to selectively disable patches.
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 first is what was failing. FWIW, I disagree on which is better. In the first example, it's easy to overlook that last argument after the function, especially when the function has enough lines. Putting each argument on its own line is the clearest way to prevent that. And, there are several cases in this file where there are 2 string arguments after the function, exacerbating the problem.
But, if you're adamant, we can avoid changing the whole file. This all started because you preferred to not change the indentation in the one of the functions that was inconsistent with the others, but had a ternary return that made it look awkward to be consistent. A possible solution for that is to do what was done elsewhere and start the function declaration on the next line. See the latest.
Includes: - **lint**: lint all code using new eslint flat config - **build**: build using rollup, minify using uglify, process for dist - **npmcopy**: copy some node module files to the external folder - **start**: watch source files and rebuild on changes using rollup.watch Also: - Update the build function in the release script to use the new build - Update CONTRIBUTING.md and README.md with new scripts - Confirm the min file was identical to the main branch build, aside from the version update. - Keep uglify-js at 3.9.4, which is the last version that officially supported the ie8 argument, which we need for ie9 - update browserstack project name to jquery-migrate - run browserstack at least once a week Closes jquerygh-512
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.
Thanks, that looks less noisy, LGTM.
As for the code style itself, I could be convinced what's better in either direction but I tend to skew towards trying to avoid changing the existing style too much, at least as long as we don't employ automatic tooling that totally enforces that.
But that would have its own drawbacks in the context of jQuery projects as I mentioned in one of my comments. And I don't currently have a good answer for that.
Git hooks are broken since jquerygh-512; this change fixes them by migrating to Husky as jQuery Core did. Without this fix, even `git commit --no-verify` doesn't work; only manually deleting the `.git/hooks` directory and re-running `git commit` does. Ref jquerygh-512
PR jquerygh-512 added the `"type": "module"` field to the top-level `package.json`, making Node-based workflows fail when trying to require Migrate as it's not exposed as ESM. To fix this, add a small `package.json` with just `"type": "commonjs"`. Also, add Node.js smoke tests as the simplest way to verify this change. Fixes jquerygh-523 Ref jquerygh-512
PR jquerygh-512 added the `"type": "module"` field to the top-level `package.json`, making Node-based workflows fail when trying to require Migrate as it's not exposed as ESM. To fix this, add a small `package.json` with just `"type": "commonjs"`. Also, add Node.js smoke tests as the simplest way to verify this change. Fixes jquerygh-523 Ref jquerygh-512
PR gh-512 added the `"type": "module"` field to the top-level `package.json`, making Node-based workflows fail when trying to require Migrate as it's not exposed as ESM. To fix this, add a small `package.json` with just `"type": "commonjs"`. Also, add Node.js smoke tests as the simplest way to verify this change. Fixes gh-523 Closes gh-525 Ref gh-512
Includes:
Also:
ie8
argument, which ironically we need for ie9Sample BrowsersStack runs:
git: https://github.com/timmywil/jquery-migrate/actions/runs/9304840969
3.x: https://github.com/timmywil/jquery-migrate/actions/runs/9305649366