Skip to content

Conversation

@BPScott
Copy link
Member

@BPScott BPScott commented Dec 4, 2021

WHY are these changes introduced?

Keeping up to date.

WHAT is this pull request doing?

  • Update eslint to v8.3.0 and @shopify/eslint-plugin to v41.0.1
  • Update babel/* rules to be @babel/*
  • Apply autofixes
  • Adjust generic naming so generic names start with a T
  • Replace usage of toBeTruthy / toBeFalsey in tests with toBeDefined / toBe(true) / toBeFalse() etc
  • Ensure key props are always before spreads to keep react/jsx-key happy
  • Ignore occasional hardcoded-content warnings
  • Add useMemo to a few places to prevent always rerendering to keep react/jsx-no-constructed-context-values happy

To Test

CI passes

@BPScott BPScott force-pushed the lint-config-updates branch from 503580a to 3e2552d Compare December 4, 2021 00:41
@BPScott BPScott changed the title Update eslint and stylelint Update eslint Dec 4, 2021
@BPScott BPScott force-pushed the lint-config-updates branch from e555bdf to aa1d7a6 Compare December 6, 2021 19:54
@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2021

size-limit report

Path Size
cjs 165.75 KB (+0.02% 🔺)
esm 96.41 KB (+0.03% 🔺)
esnext 143.1 KB (+0.02% 🔺)
css 34.34 KB (0%)

@BPScott BPScott marked this pull request as ready for review December 6, 2021 20:27
@BPScott BPScott requested a review from kyledurand December 6, 2021 20:27
@caution-tape-bot
Copy link

👋 It looks like you're updating JavaScript packages that are known
to cause problems when duplicated.

You can deduplicate them with the yarn-deduplicate utility:

npx yarn-deduplicate --packages graphql react react-dom webpack
npx yarn-deduplicate --scopes @shopify @apollo

A duplicate React version may cause an invalid hook call warning.

React context providers usually use module-scoped globals as their
default context values. Multiple versions of such packages will yield
multiple global instances, resulting in oblique runtime errors.

Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

🙌

const Wrapper = strict ? StrictMode : Fragment;
const intl = new I18n(i18n || {});
const scrollLockManager = new ScrollLockManager();
const intl = useMemo(() => new I18n(i18n || {}), [i18n]);
Copy link
Member

Choose a reason for hiding this comment

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

Necessary or optimizing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Eslint warns with react/jsx-no-constructed-context-values when you sick something in a context that shall cause the context to be updated on every single rerender regardless of if the content changes or not. With the fix being to memoize objects you pass into contexts.

In this particular case it's probably not a big deal as we probably aren't causing rerenders on PolarisTestProvider because, but it's an easy fix to write code in the way to keep the linter happy rather than sprinkling eslint-disable-next-lines around

@BPScott BPScott merged commit 741eeb6 into main Dec 7, 2021
@BPScott BPScott deleted the lint-config-updates branch December 7, 2021 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants