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: Restrict @adobe/spectrum imports #2179

Merged

Conversation

AkshatJawne
Copy link
Contributor

Closes #1908

@AkshatJawne AkshatJawne requested a review from mofojed August 1, 2024 17:55
@AkshatJawne AkshatJawne self-assigned this Aug 1, 2024
Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.66%. Comparing base (1d51585) to head (d5f9ad6).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2179      +/-   ##
==========================================
- Coverage   46.67%   46.66%   -0.01%     
==========================================
  Files         692      692              
  Lines       38620    38629       +9     
  Branches     9625     9751     +126     
==========================================
+ Hits        18025    18028       +3     
+ Misses      20584    20548      -36     
- Partials       11       53      +42     
Flag Coverage Δ
unit 46.66% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

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

This ends up removing the no import from deephaven packages rules

I think you need to combine the spectrum and self-package imports. With this config, you can do import { IrisGrid } from '@deephaven/iris-grid'; inside of IrisGrid.

@mattrunyon
Copy link
Collaborator

This also disables the self import check from the ignored paths even if the other rules are combined. Not sure the best way around that. You might be able to combine another overrides inside of the override to turn on the rule for self imports only in those files

@AkshatJawne
Copy link
Contributor Author

Not completely sure what can be done here, I have been experimenting with different configurations, but ultimately, because of the way overrides work, we can only disable the no-restricted-imports rule for a file path, not a specific import outlined in the no-restricted-imports

Gonna keep experimenting but nothing super promising so far

@AkshatJawne AkshatJawne marked this pull request as draft August 2, 2024 14:05
@AkshatJawne
Copy link
Contributor Author

I believe it should be good now @mattrunyon

@AkshatJawne AkshatJawne marked this pull request as ready for review August 2, 2024 14:19
@AkshatJawne AkshatJawne requested a review from mattrunyon August 2, 2024 14:19
Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

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

It's hard for me to comment on just the certain parts, but the config should be this

  • Prevents importing from own package everywhere including spectrum components
  • Prevents importing from @adobe/react-spectrum everywhere except spectrum components
const buildPackageManifest = require('./packageManifest');

const { packageNames, packageManifest } = buildPackageManifest();

module.exports = {
  root: true,
  extends: ['@deephaven/eslint-config'],
  ignorePatterns: ['packages/golden-layout/*', 'jest.config.*'],
  overrides: [
    {
      files: ['**/*.@(ts|tsx)'],
      parserOptions: {
        project: ['./tsconfig.eslint.json', './packages/*/tsconfig.json'],
        tsconfigRootDir: __dirname,
      },
    },
    ...packageNames.map(packageName => ({
      files: [`packages/${packageManifest.get(packageName)}/**/*.@(ts|tsx)`],
      rules: {
        'no-restricted-imports': [
          'error',
          {
            name: packageName,
            message: 'Forbid importing from owning @deephaven package.',
          },
          {
            name: '@adobe/react-spectrum',
            message:
              'Import from @deephaven/components instead of @adobe/react-spectrum.',
          },
        ],
      },
      overrides: [
        {
          files: [
            'packages/components/src/spectrum/**/*.@(ts|tsx)',
            'packages/components/src/theme/**/*.@(ts|tsx)',
          ],
          rules: {
            'no-restricted-imports': [
              'error',
              {
                name: packageName,
                message: 'Forbid importing from owning @deephaven package.',
              },
            ],
          },
        },
      ],
    })),
  ],
};

Copy link
Contributor Author

@AkshatJawne AkshatJawne left a comment

Choose a reason for hiding this comment

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

@mattrunyon On my end, when I test with the npm test:lint with my code and with the code you suggested, they both behave the same.

However, I will be pushing the change you made if you see it as more valid, I see it as more of a semantic change (the new code is a little clearer for someone to understand)

@AkshatJawne AkshatJawne requested a review from mattrunyon August 6, 2024 04:55
@mattrunyon
Copy link
Collaborator

The previous version didn't prevent importing from the owning deephaven package when in the spectrum and theme directories. It was actually preventing import a from 'packageName';

@AkshatJawne AkshatJawne merged commit a257296 into deephaven:main Aug 6, 2024
11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create an eslint rule to prevent import from @adobe/spectrum
2 participants