-
Notifications
You must be signed in to change notification settings - Fork 893
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
Improve linting and watching JS #21315
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* specify webpack src to avoid getting into a build loop * remove pre side effect: * don't enforce building CSS before * remove unknown copy command
* fix jest env troubles in JS package: not sure why the jest eslint plugin doesn't pick up the jest/globals env anymore * fix react/display name in yoastseo, even though the function is not React * ignore react/no-deprecated in to-be-remove yoast-components package
* remove obsolete --no-sort In favor of adding it yourself if wanted: * remove --stream * remove --
No longer needed now that packages transpile themselves Not sure about Parse5, so leaving that Chart.js is ignored if removing it, even though it is an entry too, not sure why
The monorepo symlinks all the packages in the node_modules, so they end up the same in the bundle
igorschoester
added
the
changelog: non-user-facing
Needs to be included in the 'Non-userfacing' category in the changelog
label
Apr 13, 2024
@igorschoester Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
Pull Request Test Coverage Report for Build e3fadd5e21d8a9c6a958f917beaa1ffc635cae55Details
💛 - Coveralls |
pls78
approved these changes
Apr 23, 2024
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.
CR && Acc: ✅
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
changelog: non-user-facing
Needs to be included in the 'Non-userfacing' category in the changelog
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
yarn start
not working andgrunt shell:webpack-watch
looping.Summary
This PR can be summarized in the following changelog entry:
yarn start
command, and it replaces thegrunt shell:webpack-watch
command.grunt shell:webpack
with eithergrunt shell:yarn:build --scope @yoast/wordpress-seo
oryarn build --scope @yoast/wordpress-seo
grunt shell:webpack-prod
with eithergrunt shell:yarn-prod:build--scope @yoast/wordpress-seo
orNODE_ENV=production yarn build --scope @yoast/wordpress-seo
grunt shell:build-packages
with eithergrunt shell:yarn:build --ignore @yoast/wordpress-seo
oryarn build --ignore @yoast/wordpress-seo
grunt shell:build-packages-prod
with eithergrunt shell:yarn-prod:build --ignore @yoast/wordpress-seo
orNODE_ENV=production yarn build --ignore @yoast/wordpress-seo
yarn lint
to also check the tooling, splitting toyarn lint:packages
andyarn lint:tooling
.yarn test
command, if you still want output like before append--stream
.import
,jsx-a11y
andreact
) to the latest versions.import
,jsx-a11y
andreact
) to the latest versions.import
,jsx-a11y
andreact
) to the latest versions.import
,jsx-a11y
andreact
) to the latest versions.import
,jsx-a11y
andreact
) to the latest versions.production
environment.Relevant technical choices:
Fix start script
Removing the
prestart
, the copy command does not exist and the the CSS is a side-effect.Specified the
--webpack-src-dir
to prevent an infinite loop from the watching.Remove JS deps from root
These are dependencies from
packages/js
?Kind of in between I suppose, because we do build
packages/js
via the root. Anddraft-js
is built as an external in the plugin build. But I thinkpackages/js
should determine which versions.Grunt tasks
Replacing specific shell commands with an abstract Yarn one so we can call any command.
Moving the NODE_ENV setting out of the scripts themselves and into the shell commands. From the terminal you can specify it yourself, but we are more precise in our Grunt tasks.
Webpack config
Since we are transpiling separately from bundling, we no longer need the exceptions inside the
node_modules
folder. And maybe some were already not needed?Adding the optimization back from the default, I am assuming that was accidentally removed by adding another key.
Removing packages check, I think this is a remnant from the
yoastseo/src
imports that we no longer have.Linting
Added linting over our tooling (at root level at least).
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
grunt build
grunt release
js/dist
)grunt watch
packages
should trigger build and linting of the packagespackages/js/tests
should trigger linting of justpackages/js
yarn test
--stream
to the commandyarn lint
yarn lint:packages
andyarn lint:tooling
yarn start
Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo]
, added test instructions for Shopify and attached theShopify
label to this PR.Documentation
Quality assurance
Innovation
innovation
label.Fixes #