-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[DataGrid] Fix webpack v5 #449
[DataGrid] Fix webpack v5 #449
Conversation
9c5e25d
to
56dbca9
Compare
@dtassone I have added a warning to edge against leaks. |
@@ -12,5 +11,5 @@ export function useApiRef(): ApiRef { | |||
const logger = useLogger('useApiRef'); | |||
logger.debug('Initializing grid api with EventEmitter.'); | |||
|
|||
return React.useRef<GridApi>(new EventEmitter()); | |||
return React.useRef(new EventEmitter() as GridApi); |
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.
why not use the Generic?
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.
Does it implement the EventEmitter type used in CoreApi?
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.
And btw it works if we use an import there
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 exactly what to do here. Without the change, TypeScript complains that the GridApi has properties that the EventEmitter instance doesn't support. Guidance from your end welcome.
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.
Ok we can pair up when you are free. Ping me on slack when it's a good time ;)
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.
Ok, let's do that tomorrow, for the sake of clarity, this is the current state of affair.
on master the type of the EventEmitter is any
, c.f. #479:
and once I rewrite the EventEmitter, it fails on:
which looks expected.
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.
Any idea? It seems that the previous type was wrong. I can't think of a better alternative.
56dbca9
to
a262d7a
Compare
@@ -12,5 +11,5 @@ export function useApiRef(): ApiRef { | |||
const logger = useLogger('useApiRef'); | |||
logger.debug('Initializing grid api with EventEmitter.'); | |||
|
|||
return React.useRef<GridApi>(new EventEmitter()); | |||
return React.useRef(new EventEmitter() as GridApi); |
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.
How about that?
return React.useRef(new EventEmitter() as GridApi); | |
return React.useRef<GridApi>(new EventEmitter() as GridApi); |
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.
Sounds great
Co-authored-by: damien tassone <damien.tassone@gmail.com>
9dcfe4d
to
7b77da2
Compare
webpack v5 does no longer polyfill Node.js modules https://webpack.js.org/blog/2020-10-10-webpack-5-release/. This fixes the component under this environment. It replaces https://github.com/browserify/events with a custom version. Help with #447.
It should also remove some bundle size, using a simplified version of the polyfill used by webpack v4.