Skip to content
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

P1-1277 - Implement tooling standards in js package #18319

Conversation

SResok
Copy link
Contributor

@SResok SResok commented Apr 7, 2022

Context

  • Implement our tooling standards in the js package
    • @yoast/babel-preset, eslint-config-yoast, @yoast/jest-preset and @yoast/browserslist-config

Summary

This PR can be summarized in the following changelog entry:

  • Implements our new tooling standards in the 'js' package

Relevant technical choices:

  • Use the transpiled code only on production build
    • Running the build command from the 'js' package on every file change when using i.e the watch command would negatively impact the development speed drastically.
  • Removed jsSrc and cssDist from config/webpack/paths.js because they were unused.
  • Decided to revert the Babel config after talking with Nolle. Seems simpler to just keep using Webpack.
    • Downside is that this picks up the Babel config in the root. However, the test still use the Babel config in the JS folder. Something to talk about at a later stage.
  • Use versions instead of file in our root
    • File copies over at the time of install. Using a version makes it a symlink because of the workspace. That seems preferable, and a must if we work on the packages.

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

  • Testing Eslint
    • Navigate to packages/js and run yarn lint
    • Verify there are now 141 warnings
  • Testing Jest
    • Navigate to packages/js and run yarn test
    • Verify Test Suites: 55 passed without any errors

Test instructions for QA when the code is in the RC

  • QA should use the same steps as above.

QA can test this PR by following these steps:

  • Check if grunt build, grunt artifact and yarn lint still work as expected.

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Other environments

  • This PR also affects Shopify. I have added a changelog entry starting with [shopify-seo], added test instructions for Shopify and attached the Shopify label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unittests to verify the code works as intended
  • If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
  • I have written this PR in accordance with my team's definition of done.

Fixes P1-1277

@SResok SResok added the changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog label Apr 7, 2022
@SResok SResok added this to the 18.7 milestone Apr 7, 2022
@SResok SResok force-pushed the P1-1277-implement-es-lint-babel-tooling-standards-in-js-package branch from 538fe2e to ddac01e Compare April 8, 2022 09:51
@SResok SResok changed the title P1-1277 - Implement EsLint & babel tooling standards in js package P1-1277 - Implement tooling standards in js package Apr 8, 2022
@SResok SResok marked this pull request as ready for review April 8, 2022 12:44
@igorschoester igorschoester force-pushed the P1-1277-implement-es-lint-babel-tooling-standards-in-js-package branch from 04549e0 to 607ac76 Compare April 11, 2022 09:58
Copy link
Member

@igorschoester igorschoester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CR & ACC ✅

Changed the approach after some discussion: no more babel build for JS package specifically. Due to it being the root JS.

@igorschoester igorschoester merged commit 7ede9f9 into trunk Apr 11, 2022
@igorschoester igorschoester deleted the P1-1277-implement-es-lint-babel-tooling-standards-in-js-package branch April 11, 2022 10:18
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants