-
Notifications
You must be signed in to change notification settings - Fork 645
feat(StateManager): create the first version #30472
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
base: 25_2
Are you sure you want to change the base?
Conversation
…r different artifact types and environments
7d1be18
to
cb87d90
Compare
Fixed "heap out of memory" errors in the Redux Devtools during the diff calculation process due to the `changes` property of `CardViewOptionsController` and `ColumnsController` controllers contains a reference to the component instance.
cb87d90
to
afdb0f6
Compare
…nals-core` wrapper
Prevent direct imports from `@preact/signals-core` package to ensure consistent state management across the codebase.
…ry` for consistency
afdb0f6
to
2eef322
Compare
@@ -0,0 +1,14 @@ | |||
export const setupStateManager = (): void => {}; |
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.
Now state_manager/index.prod.ts
and state_manager/index.ts
different only by setupStateManager
function
Also, the reactive_primitives
has the index.ts
and index.prod.ts
too
So, it's a little bit unobvious where is code that goes to prod
version and where is the dev
code
Let's make it more obvious and scalable,
I suggest to modify the state_manager
file structure the following way:
state_manager
- dev
- index.ts // <-- export of public API here
- event_emitter.ts
- logger.ts
- other files / directories related to state manager
- prod
- index.ts // <-- just re-export `@preact/signals-core` and all public API stubs for `prod` version
- index.ts // <-- root export file
@@ -0,0 +1,14 @@ | |||
export const setupStateManager = (): void => {}; | |||
|
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.
About two index
files (index.ts
& index.prod.ts
)
We can keep both of them and replace imports during build,
but it's additional logic in our complex build (build/state_manager
directory)
So, let's try to implement this feature a little bit different:
We support the ts path alias
feature with our build and already has a few defined paths:
Link to the code
So, we can create two tsconfigs and resolve paths differently for dev
and prod
builds
It looks more right way, because it's one of main reasons why the import paths was introduced
(different imports in different environments)
Only problem that gulp
takes all files by glob
(it doesn't matter these files used or not)
So we still required to change the glob
pattern depends on env
values or delete unused files by hands with separate gulp
task
# tests | ||
temp | ||
|
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.
Let's specify this gitignore
part more directly
Now it's not clear to which part of project this temp
related
I suggest to rename the temp
directory to __test-artifacts__
or sth like that to make it more obvious
@@ -25,5 +27,28 @@ const config: StorybookConfig = { | |||
autodocs: "tag", | |||
}, | |||
staticDirs: ['../stories/assets', '../node_modules/devextreme/dist'], | |||
webpackFinal: async (config, { configType }) => { |
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.
As far as I understand, the storybook
build takes already builded devextreme
To be more clear, the build chain looks the following way:
devextreme
-> devextreme-react
-> storybook
So, the storybook's webpack processed already builded libraries from deps
And it looks incorrect to "cut offthe state manager part here If we already built
devextreme` in prod mode -> it should be already cut
@@ -19,5 +19,6 @@ module.exports = { | |||
TRANSPILED_RENOVATION_PATH: 'artifacts/transpiled-renovation', | |||
TRANSPILED_PROD_ESM_PATH: 'artifacts/transpiled-esm-npm', | |||
SCSS_PACKAGE_PATH: '../devextreme-scss', | |||
EULA_URL: 'https://js.devexpress.com/Licensing/' | |||
EULA_URL: 'https://js.devexpress.com/Licensing/', | |||
REMOVE_NON_PRODUCTION_MODULE: argv.uglify, |
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.
I'm not sure that it's ok to use uglify
as flag for dev
/ prod
modes
It may lead to misunderstand in the future
As example -> I'd like to play with minified React
to reproduce the issue, and would like to use StateManager
that helps me to debug
Also, I'm not sure that in current build we have good flag to use
So, may be as a temporary solution -> let's add a separate flag
Sth like ENABLE_STATE_MANAGER
and add it to npm scripts
manually
@alexslavr cc
// eslint-disable-next-line @stylistic/max-len | ||
export class PreactSignalValueContainerManager implements StateManagementTypes.ValueContainerManager { |
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.
Let's just move implements
on the next line instead of disabling the ESLint check
// eslint-disable-next-line @stylistic/max-len | |
export class PreactSignalValueContainerManager implements StateManagementTypes.ValueContainerManager { | |
export class PreactSignalValueContainerManager | |
implements StateManagementTypes.ValueContainerManager { |
// eslint-disable-next-line @typescript-eslint/no-invalid-void-type | ||
export type EffectFn = () => void | EffectCleanup; |
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.
Let's fix linter error instead of disabling it
// eslint-disable-next-line @typescript-eslint/no-invalid-void-type | |
export type EffectFn = () => void | EffectCleanup; | |
export type EffectFn = (() => void) | EffectCleanup; |
return this.valueContainerManagers | ||
// eslint-disable-next-line @stylistic/max-len | ||
.some((currentStateContainerManager) => currentStateContainerManager.canHandle(valueContainer)); |
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.
Let's fix linter error instead of disabling it
return this.valueContainerManagers | |
// eslint-disable-next-line @stylistic/max-len | |
.some((currentStateContainerManager) => currentStateContainerManager.canHandle(valueContainer)); | |
.some(( | |
currentStateContainerManager, | |
) => currentStateContainerManager.canHandle(valueContainer)) |
// eslint-disable-next-line @stylistic/max-len | ||
.find((currentStateContainerManager) => currentStateContainerManager.canHandle(valueContainer)); |
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.
Let's fix linter error instead of disabling it
// eslint-disable-next-line @stylistic/max-len | |
.find((currentStateContainerManager) => currentStateContainerManager.canHandle(valueContainer)); | |
.find(( | |
currentStateContainerManager, | |
) => currentStateContainerManager.canHandle(valueContainer)); |
onExternalAction: (callback: DevToolsExternalActionCallback) => void; | ||
} | ||
|
||
// eslint-disable-next-line spellcheck/spell-checker |
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.
Let's disable spell-checker
linter errors on top of the file
/* eslint-disable spellcheck/spell-checker */
Unfortunately, we don't update this rule and don't add new values (for a long time)
So, for now it's ok to disable it for files where a lot of spell-checker
errors occures
@IlyaKhD, @pomahtri maybe we should disable spell-checker
rule from our default config?
Or let's create a tech task to actualize it and all valid exceptions :)
No description provided.