Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

refactor: import value name from root of superset-ui/core #17947

Merged
merged 13 commits into from
Jan 8, 2022

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Jan 6, 2022

SUMMARY

Currently, there are multiple ways to import a value name from superset-ui/core, this confusing import expression makes the codebase difficult to maintain.

You can use following approach in superset-frontend/src with Webpack, the config at here

  1. import { ensureIsArray } from '@superset-ui/core'

You can use following approach in superset-frontend/packages/superset-ui-core/test with Jest and Eslint, the config at here(Jest) and here(Eslint).

  1. import { ensureIsArray } from '@superset-ui/core'
  2. import { ensureIsArray } from '@superset-ui/core/src/utils/ensureIsArray'
  3. import { ensureIsArray } from '../../some/relative/path'

This PR enforces the codebase using unified import expression: import { ensureIsArray } from '@superset-ui/core'. If you want to import an internal(private) module from '@superset-ui/core', you should use relative import and export the corresponding value name.

after the PR, eslint rules will prevent following import expression

  1. import { ensureIsArray } from '@superset-ui/core/src/some/path
  2. import { testValueName } from '@superset-ui/core/test/some/path
  3. import { valueNameFromLib } from @superset-ui/core/lib/some/path

after the PR, eslint rules will approve following import expression

  1. ✔️ import { ensureIsArray } from '@superset-ui/core'

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

CI

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@zhaoyongjie zhaoyongjie changed the title refactor: import superset-ui/core from root of module refactor: import module from root of superset-ui/core Jan 6, 2022
@codecov
Copy link

codecov bot commented Jan 6, 2022

Codecov Report

Merging #17947 (50a694d) into master (79b6dc2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #17947   +/-   ##
=======================================
  Coverage   67.07%   67.07%           
=======================================
  Files        1609     1609           
  Lines       64905    64905           
  Branches     6868     6868           
=======================================
  Hits        43537    43537           
  Misses      19502    19502           
  Partials     1866     1866           
Flag Coverage Δ
javascript 53.77% <ø> (ø)

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


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79b6dc2...50a694d. Read the comment docs.

@zhaoyongjie zhaoyongjie changed the title refactor: import module from root of superset-ui/core refactor: import value name from root of superset-ui/core Jan 6, 2022
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up, this will ensure a much nicer development experience!

@michael-s-molina
Copy link
Member

Love it!

import { LOGIN_GLOB } from '../fixtures/constants';
import { sankeyFormData } from '../fixtures/formData';
import { SliceIdAndOrFormData } from '../../../src/chart/clients/ChartClient';
Copy link
Member

@michael-s-molina michael-s-molina Jan 7, 2022

Choose a reason for hiding this comment

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

Since the packages are self-contained, is there a way to set something in each package.json or globally to make packages/<package-name> as the root for each package and avoid the ../../.. imports?

Copy link
Member Author

@zhaoyongjie zhaoyongjie Jan 7, 2022

Choose a reason for hiding this comment

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

IMO, have to keep a simple and straightforward approach to link modules. We have to consider multiple runtime to alias these module

  • Eslint
  • Jest
  • Webpack

This pattern only keep 1 principle "public variable name import from top level"

What do you think about it?

@zhaoyongjie zhaoyongjie merged commit cb97e37 into apache:master Jan 8, 2022
shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
* chart

* index and number-format

* char-composition and color

* connection

* dimension and math-expression

* models

* query

* time-format

* residual modules

* change config of jest and eslint

config

* wip

update package.json

* fix lint

* WithLegend import from superset-ui/core
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
* chart

* index and number-format

* char-composition and color

* connection

* dimension and math-expression

* models

* query

* time-format

* residual modules

* change config of jest and eslint

config

* wip

update package.json

* fix lint

* WithLegend import from superset-ui/core
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants