Skip to content
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

adjustments to webpack to improve build speed #281

Merged
merged 11 commits into from
Apr 22, 2024

Conversation

BSd3v
Copy link
Collaborator

@BSd3v BSd3v commented Apr 4, 2024

adjustments to webpack to improve build speed

@BSd3v BSd3v requested review from alexcjohnson and ndrezn April 4, 2024 22:58
@@ -57,12 +58,15 @@ module.exports = (env, argv) => {
module: {
rules: [
{
exclude: /node_modules/,
Copy link
Member

Choose a reason for hiding this comment

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

@alexcjohnson, curious what your thought on this exclusion is?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it's normal to exclude node_modules as much as you can. In some cases we've had to explicitly include certain external modules in the babel transpilation step - see eg dcc - because those modules have started distributing their code using more modern JS than we accept in our bundles.

But if we're sure the es-check command included as postbuild in our package.json is running as part of CI and succeeding, then this is fine. It's currently set to es2017 which fits our general intent to support 7-year-old browsers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As a note, AG Grid looks to only support the two latest major browsers versions.

https://www.ag-grid.com/react-data-grid/supported-browsers/

This might cause some conflicts eventually.

@AnnMarieW
Copy link
Collaborator

One more minor (and unrelated) item I found when I was updating the docs recently:

Console msg:

ag-grid-community.auto.esm.js:484 AG Grid: setDatasource is deprecated. Please use 'api.setGridOption('datasource', newValue)' or 'api.updateGridOptions({ datasource: newValue })' instead.

params.api.setDatasource(this.getDatasource());

Do you think you could include this small update in this PR?

@BSd3v
Copy link
Collaborator Author

BSd3v commented Apr 12, 2024

Oh. Interesting.

Is it to be declared as a prop? The name didn't change...

@AnnMarieW
Copy link
Collaborator

It would be nice to still handle this in the component rather than the dash app. Just needs to be set in the gridOptions ?

@BSd3v
Copy link
Collaborator Author

BSd3v commented Apr 13, 2024

Yeah. It would be in the component.

@ndrezn ndrezn mentioned this pull request Apr 16, 2024
Copy link
Member

@ndrezn ndrezn left a comment

Choose a reason for hiding this comment

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

🚀 Looks great.

I am seeing:

tests/test_cell_data_type_override.py::test_cd001_cell_data_types_override FAILED                      [  5%]

E           selenium.common.exceptions.TimeoutException: Message: text -> 17/01/2024 not found within 10s, found: 04/02/11720

and:

tests/test_popupparent.py::test_pp001_popupParent FAILED                                               [ 60%]

E       selenium.common.exceptions.NoSuchElementException: Message: no such element: Unable to locate element: {"method":"css selector","selector":"body > .ag-popup .mantine-Select-input"}

failing locally but I believe this is unrelated to this PR.

This is a great change! I'm seeing on my M1 Pro:

npm run build  10.78s user 1.12s system 167% cpu 7.114 total

And a brutal:

npm run build  233.24s user 13.03s system 130% cpu 3:09.10 total

on main. Good stuff thank you @BSd3v!

@ndrezn ndrezn merged commit 434bd13 into plotly:main Apr 22, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants