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.

Sign up for GitHub

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

Jump to bottom

Optimizing the chunk size using devtool #1520

Merged
merged 1 commit into from
Aug 14, 2024
Merged

Optimizing the chunk size using devtool #1520

merged 1 commit into from
Aug 14, 2024

Conversation

GowthamShanmugam
Copy link
Contributor

@GowthamShanmugam GowthamShanmugam commented Aug 6, 2024 •
edited
Loading

GowthamShanmugam
GowthamShanmugam commented Aug 6, 2024
View reviewed changes
webpack.config.ts Outdated
// 'named' Readable ids for better debugging.
chunkIds: isProduction ? 'deterministic' : 'named',
},
devServer: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved devServer at the bottom for better readability

SanjalKatiyar
SanjalKatiyar reviewed Aug 6, 2024
View reviewed changes
package.json Outdated
Comment on lines 12 to 15
"clean": "yarn clean-mco && yarn clean-odf",
"clean-mco": "cd plugins/mco && rm -rf ./dist",
"clean-odf": "cd plugins/odf && rm -rf ./dist",
"clean-client": "cd plugins/client && rm -rf ./dist",
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

why we removing this ?? We use them atleast on local.

Copy link
Contributor Author

@GowthamShanmugam GowthamShanmugam Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

webpack config: clean: true, will takecare of this for both Prod and dev

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The clean config is not working always, I am also facing same issue:webpack/webpack-dev-middleware#861

i will revert this change

SanjalKatiyar
SanjalKatiyar reviewed Aug 6, 2024
View reviewed changes
webpack.config.ts Outdated
Comment on lines 41 to 44
filename: '[name].bundle.[contenthash:8].js',
chunkFilename: '[name].[contenthash:8].js',
// Clean the output directory(dist) before emit
clean: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this might be not enough to enable caching... manifest file (plugin metadata) has a constant name which can get cached, IIRC console rely on version field to prevent that.
We need to confirm with OCP team though once, whether that fix will work or not, then update our version accordingly for main and compatibility builds (if not already).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

Copy link
Contributor Author

@GowthamShanmugam GowthamShanmugam Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Validation on the odf-console side:
After the base path is changed the version is updated on the manifest json,
For the main branch: "version": "4.17.0",
For the compatibility branch: "version": "4.17.0-compatibility",

So we are good for further testing. Will check with OCP team and update here.

SanjalKatiyar
SanjalKatiyar reviewed Aug 6, 2024
View reviewed changes
webpack.config.ts
@@ -153,13 +144,34 @@ const config: webpack.Configuration & DevServerConfiguration = {
cwd: process.cwd(),
}),
],
devtool: 'eval-cheap-module-source-map',
// 'source-map' is recommended choice for production builds, A full SourceMap is emitted as a separate file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't need source for production, it will unnecessary increase the bundle size...

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe I am missing something here ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the same but one corner case here is, in case of any bug in production we don't get any stack trace for debugging the issue.

@GowthamShanmugam GowthamShanmugam mentioned this pull request Aug 6, 2024
Open
@GowthamShanmugam
Copy link
Contributor Author

/test odf-console-e2e-aws

@SanjalKatiyar
Copy link
Collaborator

@GowthamShanmugam if we are planning to backport this PR to 4.16.z can we remove hashing and other unrelated changes from this PR and add them to #1525 instead ?? Let's keep this PR short with only what's needed (devTool part), wdyt ??

In case this is 4.17 onwards fix then we are good.

@GowthamShanmugam
Copy link
Contributor Author

@GowthamShanmugam if we are planning to backport this PR to 4.16.z can we remove hashing and other unrelated changes from this PR and add them to #1525 instead ?? Let's keep this PR short with only what's needed (devTool part), wdyt ??

In case this is 4.17 onwards fix then we are good.

ack, that's a good suggestion

@GowthamShanmugam
Optimize chunk size by changing devtool
eeb5882 
Signed-off-by: Gowtham Shanmugasundaram <[email protected]>
@GowthamShanmugam GowthamShanmugam changed the title Modified webpack.config.ts for the better performance Optimizing the chunk size using devtool Aug 7, 2024
@SanjalKatiyar SanjalKatiyar mentioned this pull request Aug 12, 2024
Open
@SanjalKatiyar
Copy link
Collaborator

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 14, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Aug 14, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GowthamShanmugam, SanjalKatiyar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@SanjalKatiyar
Copy link
Collaborator

/cherry-pick release-4.17

@openshift-cherrypick-robot
Copy link

@SanjalKatiyar: once the present PR merges, I will cherry-pick it on top of release-4.17 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.17

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@SanjalKatiyar
Copy link
Collaborator

/cherry-pick release-4.17-compatibility

@openshift-cherrypick-robot
Copy link

@SanjalKatiyar: once the present PR merges, I will cherry-pick it on top of release-4.17-compatibility in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.17-compatibility

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@SanjalKatiyar
Copy link
Collaborator

/cherry-pick release-4.16

@openshift-cherrypick-robot
Copy link

@SanjalKatiyar: once the present PR merges, I will cherry-pick it on top of release-4.16 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.16

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@SanjalKatiyar
Copy link
Collaborator

/cherry-pick release-4.16-compatibility

@openshift-cherrypick-robot
Copy link

@SanjalKatiyar: once the present PR merges, I will cherry-pick it on top of release-4.16-compatibility in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.16-compatibility

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@GowthamShanmugam
Copy link
Contributor Author

/test odf-console-e2e-aws

@openshift-merge-bot openshift-merge-bot bot merged commit 3681bf0 into red-hat-storage:master Aug 14, 2024
5 checks passed
@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Aug 14, 2024
Closed
@openshift-cherrypick-robot
Copy link

@SanjalKatiyar: new pull request created: #1534

In response to this:

/cherry-pick release-4.17

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Aug 14, 2024
Closed
@openshift-cherrypick-robot
Copy link

@SanjalKatiyar: new pull request created: #1536

In response to this:

/cherry-pick release-4.17-compatibility

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-cherrypick-robot
Copy link

@SanjalKatiyar: #1520 failed to apply on top of branch "release-4.16":

Applying: Optimize chunk size by changing devtool
Using index info to reconstruct a base tree...
M	webpack.config.ts
Falling back to patching base and 3-way merge...
Auto-merging webpack.config.ts
CONFLICT (content): Merge conflict in webpack.config.ts
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Optimize chunk size by changing devtool
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.16

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-cherrypick-robot
Copy link

@SanjalKatiyar: #1520 failed to apply on top of branch "release-4.16-compatibility":

Applying: Optimize chunk size by changing devtool
Using index info to reconstruct a base tree...
M	webpack.config.ts
Falling back to patching base and 3-way merge...
Auto-merging webpack.config.ts
CONFLICT (content): Merge conflict in webpack.config.ts
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Optimize chunk size by changing devtool
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.16-compatibility

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SanjalKatiyar SanjalKatiyar SanjalKatiyar left review comments

Assignees

@SanjalKatiyar SanjalKatiyar

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GowthamShanmugam @SanjalKatiyar @openshift-cherrypick-robot