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

fix(core): update eslintrc config to show no-restricted-imports rules #7367

Merged
merged 3 commits into from
Aug 14, 2024

Conversation

pedrobonamin
Copy link
Contributor

@pedrobonamin pedrobonamin commented Aug 13, 2024

Description

The existing eslint config is overriding the original no-restricted-imports rule.
When this rule was added:

'no-restricted-imports': [
          'error',
          {
            paths: [
              {
                name: 'react',
                importNames: ['createContext'],
                message: 'Please place context in _singletons',
              },
            ],
          },

The previous rules stopped working.

  • This PR introduces changes to make all the no-restricted-imports rules work at the same time.
  • This PR also fixes the failing checks, by either importing the corresponding files or opting out where needed.

With the changes introduced, now we are seeing all the expected errors.
Screenshot 2024-08-14 at 09 55 18

What to review

  • Are this changes ok?
  • Is there any way to configure the eslint file to expose different error messages for React default import error and context error ?
  • Small caveat: The error for "Please place context in _singletons" is not standalone anymore, it's shared with the React default error.

Testing

Create a .tsx file inside packages/sanity/* and add the following code, the errors should show in the file and when running the lint check action.

import {Button} from '@sanity/ui'
import React, {createContext} from 'react'
import styled from 'styled-components'

const StyledButton = styled(Button)`
  padding: 10px;
`
const context = createContext(null)

export function Test() {
  const [count, setCount] = React.useState(0)
  return <StyledButton onClick={() => setCount(count + 1)} text={`count: ${count}`} />
}

Notes for release

Copy link

vercel bot commented Aug 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
page-building-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 14, 2024 8:00am
performance-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 14, 2024 8:00am
test-compiled-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 14, 2024 8:00am
test-next-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 14, 2024 8:00am
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 14, 2024 8:00am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Aug 14, 2024 8:00am

Copy link
Contributor

No changes to documentation

Copy link
Contributor

github-actions bot commented Aug 13, 2024

Component Testing Report Updated Aug 14, 2024 8:07 AM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 45s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 8s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 30s 6 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 36s 11 7 0
formBuilder/inputs/PortableText/copyPaste/CopyPasteFields.spec.tsx ✅ Passed (Inspect) 0s 0 12 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 17s 6 0 0
formBuilder/inputs/PortableText/DisableFocusAndUnset.spec.tsx ✅ Passed (Inspect) 10s 3 0 0
formBuilder/inputs/PortableText/DragAndDrop.spec.tsx ✅ Passed (Inspect) 3m 0s 0 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 44s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 1m 43s 21 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 15s 18 0 0
formBuilder/inputs/PortableText/PresenceCursors.spec.tsx ✅ Passed (Inspect) 8s 3 9 0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx ✅ Passed (Inspect) 25s 9 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 18s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ❌ Failed (Inspect) 1m 22s 20 0 1
formBuilder/tree-editing/TreeEditing.spec.tsx ✅ Passed (Inspect) 1m 47s 30 0 0
formBuilder/tree-editing/TreeEditingNestedObjects.spec.tsx ✅ Passed (Inspect) 20s 3 0 0

@pedrobonamin pedrobonamin marked this pull request as ready for review August 14, 2024 06:52
@pedrobonamin pedrobonamin requested review from a team as code owners August 14, 2024 06:52
@pedrobonamin pedrobonamin requested review from jordanl17 and juice49 and removed request for a team August 14, 2024 06:52
@@ -138,7 +139,6 @@ export function TreeEditingSearch(props: TreeEditingSearchProps): JSX.Element {

return (
<StyledPopover
animate
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the default in the ui-components popover

@@ -219,6 +225,7 @@ export function TreeEditingDialog(props: TreeEditingDialogProps): JSX.Element |
<Card borderTop>
<Flex align="center" justify="flex-end" paddingX={3} paddingY={2} sizing="border">
<Button
size="large"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The large ui-components button equals to the default size of the button imported from @sanity/ui

Copy link
Contributor

@RitaDias RitaDias left a comment

Choose a reason for hiding this comment

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

Tested and double checked with the modal and everything looks good :)
The only thing I don't know a whole lot about is the setting up of the linting but it looks good to me as well!

Copy link
Contributor

@binoy14 binoy14 left a comment

Choose a reason for hiding this comment

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

Good catch! thanks for fixing it

@binoy14 binoy14 added this pull request to the merge queue Aug 14, 2024
Merged via the queue into next with commit 22e717e Aug 14, 2024
42 checks passed
@binoy14 binoy14 deleted the eslintrc-fix branch August 14, 2024 14:21
cngonzalez pushed a commit that referenced this pull request Aug 20, 2024
…#7367)

* fix(core): update eslintrc config to show no-restricted-imports rules

* fix(core): change rule path to its original value
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.

3 participants