refactor: move translations and logging to new core #36929
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
msyavuz
force-pushed
the
msyavuz/chore/move-translator
branch
from
be62782 to
1d96bc7
Compare
msyavuz
force-pushed
the
msyavuz/chore/move-translator
branch
from
5d3d9bc to
e8b1b24
Compare
msyavuz
requested review from
eschutho,
geido,
kgabryje,
michael-s-molina,
rusackas and
villebro
as code owners
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| */ | ||
| import { ReactNode } from 'react'; | ||
| import { JsonValue, t } from '@superset-ui/core'; | ||
| import { t } from '@apache-superset/core'; |
There was a problem hiding this comment.
Suggestion: Importing the translation helper from the application-specific module instead of the shared UI core module couples this reusable chart-controls package to the app bundle and can cause runtime module resolution errors (or missing peer dependency issues) when @superset-ui/chart-controls is used outside the main Superset frontend; it should continue to import t from the shared @superset-ui/core package instead. [possible bug]
Severity Level: Critical 🚨
| import { t } from '@apache-superset/core'; | |
| import { t } from '@superset-ui/core'; |
Why it matters? ⭐
The PR's final file imports t from '@apache-superset/core', which is an application-specific package.
This chart-controls package is intended to be a reusable library; depending on the app package will
couple the library to the superset frontend build and can cause resolution/peer-dependency issues for
external consumers. Importing t from '@superset-ui/core' (the shared UI core) is the correct choice
for a reusable package and fixes a real packaging/runtime coupling problem rather than being merely
cosmetic.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/components/RadioButtonControl.tsx
**Line:** 20:20
**Comment:**
*Possible Bug: Importing the translation helper from the application-specific module instead of the shared UI core module couples this reusable chart-controls package to the app bundle and can cause runtime module resolution errors (or missing peer dependency issues) when `@superset-ui/chart-controls` is used outside the main Superset frontend; it should continue to import `t` from the shared `@superset-ui/core` package instead.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment.
@superset-ui/chart-controls declares @apache-superset/core as its dependency.
Nitpicks 🔍
|
michael-s-molina
left a comment
michael-s-molina
left a comment
There was a problem hiding this comment.
Thank you for the PR @msyavuz. There are some small tweaks to test and README files but let's do them in a follow-up due to the high potential for conflicts in this PR.
| jest.resetAllMocks(); | ||
| }); | ||
|
|
||
| it('should pipe to `console` methods', () => { |
There was a problem hiding this comment.
| it('should pipe to `console` methods', () => { | |
| test('should pipe to `console` methods', () => { |
| expect(() => logging.trace()).toThrow('Trace:'); | ||
| }); | ||
|
|
||
| it('should use noop functions when console unavailable', () => { |
There was a problem hiding this comment.
| it('should use noop functions when console unavailable', () => { | |
| test('should use noop functions when console unavailable', () => { |
| */ | ||
| import UntypedJed from 'jed'; | ||
| import logging from '../utils/logging'; | ||
| import logging from '../../utils/logging'; |
There was a problem hiding this comment.
The README file is still referencing @superset-ui/core.
michael-s-molina
changed the title
There was a problem hiding this comment.
Pull request overview
This PR moves translation (t, tn, configure) and logging functions from @superset-ui/core to the new @apache-superset/core package as part of the extensions project. The changes update import statements across the codebase to use the new package location.
Changes:
- Translation functions moved to
@apache-superset/corewith updated imports across ~300+ files - Logging utility moved to
@apache-superset/corewith corresponding import updates - Test files relocated and updated to reflect new package structure
Reviewed changes
Copilot reviewed 293 out of 704 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| superset-frontend/packages/superset-ui-core/src/index.ts | Removed translation exports from old core package |
| superset-frontend/packages/superset-core/src/index.ts | Added utils exports to new core package |
| superset-frontend/packages/superset-core/src/ui/index.ts | Added translation exports to UI module |
| superset-frontend/spec/helpers/shim.tsx | Updated translation configuration import path |
| superset-frontend/packages/superset-ui-core/test/utils/featureFlag.test.ts | Updated imports to use new core package for logging |
| All SqlLab files | Updated translation imports from @superset-ui/core to @apache-superset/core |
| All plugin files | Updated translation imports to new package location |
| All validator files | Updated translation imports for error messages |
| expect(uiCore.isFeatureEnabled(uiCore.FeatureFlag.DrillBy)).toEqual(false); | ||
| expect(isFeatureEnabled(FeatureFlag.DrillBy)).toEqual(false); | ||
| expect(uiCore.logging.error).toHaveBeenCalled(); | ||
| expect(logging).toHaveBeenCalledWith('Failed to query feature flag DRILL_BY'); |
There was a problem hiding this comment.
Line 61 references an undefined variable logging. This should be uiCore.logging.error to match the mock being tested on line 60.
| expect(logging).toHaveBeenCalledWith('Failed to query feature flag DRILL_BY'); | |
| expect(uiCore.logging.error).toHaveBeenCalledWith('Failed to query feature flag DRILL_BY'); |
| */ | ||
| import UntypedJed from 'jed'; | ||
| import logging from '../utils/logging'; | ||
| import logging from '../../utils/logging'; |
There was a problem hiding this comment.
The import path uses relative paths across multiple directory levels (../../). Consider using an absolute import from the package root for better maintainability and clarity.
| import logging from '../../utils/logging'; | |
| import logging from 'src/utils/logging'; |
| jest.mock('@apache-superset/core/ui', () => ({ | ||
| ...jest.requireActual('@apache-superset/core/ui'), |
There was a problem hiding this comment.
The mock is now targeting the entire @apache-superset/core/ui module instead of the more specific theme/utils/themeUtils path. This broader mock could hide issues with other exports from the ui module. Consider maintaining the more specific mock target if possible.
| jest.mock('@apache-superset/core/ui', () => ({ | |
| ...jest.requireActual('@apache-superset/core/ui'), | |
| jest.mock('@apache-superset/core/ui/theme/utils/themeUtils', () => ({ | |
| ...jest.requireActual('@apache-superset/core/ui/theme/utils/themeUtils'), |
|
|
||
| /** | ||
| * Superset frontend logger, currently just an alias to console. | ||
| * Superset logger, currently just an alias to console. |
There was a problem hiding this comment.
The comment was updated from 'Superset frontend logger' to 'Superset logger', but this is still in the frontend package. The comment should clarify that this is for frontend logging to avoid confusion.
| * Superset logger, currently just an alias to console. | |
| * Superset frontend logger, currently just an alias to the browser console. |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
LGTM! The only thing I'll mention (for this or a follow-up PR) is that Storybook stories for modules/components available to extensions show up in the docs. We should make sure that Extension-available components all have storybook entries, but also I'm wondering if certain utils like t() or logging - while not components - are storybook-worthy in that way as well.... it's a bit odd, but might make sense.
|
I wonder if it's worth documenting somewhere WHY we use this logging service... it's partly to make sure console calls get alerted/removed by developers, but mainly for safety, where not all browsers have all these methods. Most devs probably don't even know about console.table, but this allows us to use it freely... which we should probably do more often ;) |
msyavuz
force-pushed
the
msyavuz/chore/move-translator
branch
from
6626e8f to
fc4a248
Compare
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
3 participants
SUMMARY
Move translation function and logging from @superset-ui/core to new @apache-superset/core as part of the extensions project.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Translations and logs should work across the app and be available in extensions
ADDITIONAL INFORMATION