-
Notifications
You must be signed in to change notification settings - Fork 16.5k
refactor: move translations and logging to new core #36929
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
Changes from all commits
2602a6a
9a4e007
8a337c7
04fd253
a6026f8
d9cbfa9
eb8dbdb
85efe0f
7a15c8d
4e372a2
f24b99b
4736d3e
49457f2
fc4a248
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,3 +18,4 @@ | |
| */ | ||
| export * from './api'; | ||
| export * from './ui'; | ||
| export * from './utils'; | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,69 @@ | ||||||
| /** | ||||||
| * Licensed to the Apache Software Foundation (ASF) under one | ||||||
| * or more contributor license agreements. See the NOTICE file | ||||||
| * distributed with this work for additional information | ||||||
| * regarding copyright ownership. The ASF licenses this file | ||||||
| * to you under the Apache License, Version 2.0 (the | ||||||
| * "License"); you may not use this file except in compliance | ||||||
| * with the License. You may obtain a copy of the License at | ||||||
| * | ||||||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||||||
| * | ||||||
| * Unless required by applicable law or agreed to in writing, | ||||||
| * software distributed under the License is distributed on an | ||||||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||||||
| * KIND, either express or implied. See the License for the | ||||||
| * specific language governing permissions and limitations | ||||||
| * under the License. | ||||||
| */ | ||||||
| beforeEach(() => { | ||||||
| jest.resetModules(); | ||||||
| jest.resetAllMocks(); | ||||||
| }); | ||||||
|
|
||||||
| it('should pipe to `console` methods', () => { | ||||||
| const { logging } = require('@apache-superset/core'); | ||||||
|
|
||||||
| jest.spyOn(logging, 'debug').mockImplementation(); | ||||||
| jest.spyOn(logging, 'log').mockImplementation(); | ||||||
| jest.spyOn(logging, 'info').mockImplementation(); | ||||||
| expect(() => { | ||||||
| logging.debug(); | ||||||
| logging.log(); | ||||||
| logging.info(); | ||||||
| }).not.toThrow(); | ||||||
|
|
||||||
| jest.spyOn(logging, 'warn').mockImplementation(() => { | ||||||
| throw new Error('warn'); | ||||||
| }); | ||||||
| expect(() => logging.warn()).toThrow('warn'); | ||||||
|
|
||||||
| jest.spyOn(logging, 'error').mockImplementation(() => { | ||||||
| throw new Error('error'); | ||||||
| }); | ||||||
| expect(() => logging.error()).toThrow('error'); | ||||||
|
|
||||||
| jest.spyOn(logging, 'trace').mockImplementation(() => { | ||||||
| throw new Error('Trace:'); | ||||||
| }); | ||||||
| expect(() => logging.trace()).toThrow('Trace:'); | ||||||
| }); | ||||||
|
|
||||||
| it('should use noop functions when console unavailable', () => { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| Object.assign(window, { console: undefined }); | ||||||
| const { logging } = require('@apache-superset/core'); | ||||||
|
|
||||||
| expect(() => { | ||||||
| logging.debug(); | ||||||
| logging.log(); | ||||||
| logging.info(); | ||||||
| logging.warn('warn'); | ||||||
| logging.error('error'); | ||||||
| logging.trace(); | ||||||
| logging.table([ | ||||||
| [1, 2], | ||||||
| [3, 4], | ||||||
| ]); | ||||||
| }).not.toThrow(); | ||||||
| Object.assign(window, { console }); | ||||||
| }); | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,3 +18,4 @@ | |
| */ | ||
| export * from './theme'; | ||
| export * from './components'; | ||
| export * from './translation'; | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -17,7 +17,7 @@ | |||||
| * under the License. | ||||||
| */ | ||||||
| import UntypedJed from 'jed'; | ||||||
| import logging from '../utils/logging'; | ||||||
| import logging from '../../utils/logging'; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The README file is still referencing
|
||||||
| import logging from '../../utils/logging'; | |
| import logging from 'src/utils/logging'; |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -31,7 +31,7 @@ const logger = { | |||||
| }; | ||||||
|
|
||||||
| /** | ||||||
| * Superset frontend logger, currently just an alias to console. | ||||||
| * Superset logger, currently just an alias to console. | ||||||
|
||||||
| * Superset logger, currently just an alias to console. | |
| * Superset frontend logger, currently just an alias to the browser console. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -17,7 +17,8 @@ | |||||
| * under the License. | ||||||
| */ | ||||||
| import { ReactNode } from 'react'; | ||||||
| import { JsonValue, t } from '@superset-ui/core'; | ||||||
| import { t } from '@apache-superset/core'; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Severity Level: Critical 🚨
Suggested change
Why it matters? ⭐The PR's final file imports t from '@apache-superset/core', which is an application-specific package. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
| import { JsonValue } from '@superset-ui/core'; | ||||||
| import { Radio } from '@superset-ui/core/components'; | ||||||
| import { ControlHeader } from '../../components/ControlHeader'; | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.