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

[DDW-596] webpack 5 upgrade #2772

Merged
merged 200 commits into from
Jun 21, 2022
Merged

Conversation

danielmain
Copy link
Contributor

@danielmain danielmain commented Dec 3, 2021

This PR updates Webpack 4 to 5 and introduces some DX improvements.

Todos

  • Upgrade Webpack and related dependencies (Dominik)
  • Get Webpack 5 to build our project (Dominik)
  • Improve yarn dev mode with react fast-refresh (Dominik)
  • Fix small bug with maintaining window size on page reloads (Dominik)
    How to reproduce: move or resize the app window and reload the page with cmd + R -> the window jumps back into the previous place
  • Fix broken test environment (Dominik)
  • Update sass-loader and move from node-sass (deprecated) to dart-sass (Przemysław Włodek)
  • fix broken storybook (Dominik)
  • fix failing tests (Dominik)
  • Make CI builds pass (@michalrus )

Screenshots

  • React fast-refresh for "instant" CSS changes:
css-fast-refresh.mp4
  • React fast-refresh for "instant" component changes:
fast-refresh-components.mp4
  • Any changes in the renderer outside of components will result in a page reload like previously.
  • Any changes in the main process files will reload the whole app

Testing Checklist

  • Slack QA thread
  • Since this changes our build process it potentially affects all areas of our codebase and thorough QA testing is required
  • Especially: Test if maintaining window size feature still works the same as before (+ bug with reloading page after change is fixed)

Review Checklist

Basics

  • PR assigned to the PR author(s)
  • input-output-hk/daedalus-dev and input-output-hk/daedalus-qa assigned as PR reviewers
  • If there are UI changes, Alexander Rukin assigned as an additional reviewer
  • All visual regression testing has been reviewed (assign run Chromatic label to PR to trigger the run)
  • PR has appropriate labels (release-vNext, feature/bug/chore, WIP)
  • PR link is added to a Jira ticket, ticket moved to In Review
  • PR is updated to the most recent version of the target branch (and there are no conflicts)
  • PR has a good description that summarizes all changes
  • PR contains screenshots (in case of UI changes)
  • CHANGELOG entry has been added to the top of the appropriate section (Features, Fixes, Chores) and is linked to the correct PR on GitHub
  • There are no missing translations (running yarn manage:translations produces no changes)
  • Text changes are proofread and approved (Jane Wild / Amy Reeve)
  • Japanese text changes are proofread and approved (Junko Oda)
  • Storybook works and no stories are broken (yarn storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

  • Important parts of the code are properly commented and documented
  • Code is properly typed with typescript types
  • React components are split-up enough to avoid unnecessary re-renderings
  • Any code that only works in main process is neatly separated from components

Testing

  • New feature/change is covered by acceptance tests
  • New feature/change is manually tested and approved by QA team
  • All existing acceptance tests are still up-to-date
  • New feature/change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review

  • Update Slack QA thread by marking it with a green checkmark

@danielmain danielmain added the WIP label Dec 3, 2021
@michalrus michalrus force-pushed the chore/ddw-596-webpack-5-upgrade branch from fdde1bf to 1890cd1 Compare June 6, 2022 13:16
@miorsufianiohk miorsufianiohk self-requested a review June 14, 2022 08:33
@shawnbusuttil shawnbusuttil self-requested a review June 14, 2022 12:46
Copy link
Contributor

@renanvalentin renanvalentin left a comment

Choose a reason for hiding this comment

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

Really amazing job!

Comment on lines +87 to +88
if (existingChannel)
throw new Error(`Channel ${channelName} already exists`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make sure this will not result in any side-effects?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this related to the upgrade?

Copy link
Member

Choose a reason for hiding this comment

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

This is fixing an eslint error:
grafik

source/main/preload.ts Show resolved Hide resolved
source/main/webpack.config.js Show resolved Hide resolved
@@ -77,7 +80,11 @@ export const createMainWindow = (locale: string, windowBounds?: Rectangle) => {
if (event.sender !== window.webContents) return;
window.close();
});
window.loadURL(`file://${__dirname}/../renderer/index.html`);
if (isDev) {
window.loadURL(`http://localhost:8080`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have localhost hard coded?

Copy link
Member

Choose a reason for hiding this comment

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

Because that's the webpack dev server - but i agree we could pass this as env var into the main process 👍 but i would do that in a separate PR tbh

@@ -0,0 +1,158 @@
import React, { useState, useCallback, useMemo } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

File moved to source/renderer/app/components/wallet/tokens/wallet-tokens/WalletTokens.tsx

@@ -0,0 +1,232 @@
import React, { useState, useMemo, useCallback } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

File moved to source/renderer/app/components/wallet/tokens/wallet-tokens-list/WalletTokensList.tsx

source/renderer/app/stores/AppUpdateStore.ts Show resolved Hide resolved
<body>
<div id="root"></div>
</body>
</html>
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should have an .http extension as it does not use any templating feature.

Copy link
Member

Choose a reason for hiding this comment

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

you mean .html right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.
.http - haha 🤣

Copy link

@miorsufianiohk miorsufianiohk left a comment

Choose a reason for hiding this comment

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

LGTM on all 4 builds: intel Mac, M1 Mac, Linux and Windows. Tested on 21914. Great work everyone 👍 (ps: Great job on fixing the M1 build @michalrus )

@danielmain danielmain merged commit 35f93ba into develop Jun 21, 2022
@iohk-bors iohk-bors bot deleted the chore/ddw-596-webpack-5-upgrade branch June 21, 2022 09:54
@michalrus
Copy link
Member

Thank you, everyone! 🙇 ✨

@DominikGuzei
Copy link
Member

A miracle happened 🎉 thanks everyone for your precious contributions!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.