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

hmr script #50

Merged
merged 17 commits into from
Jun 27, 2024
Merged

hmr script #50

merged 17 commits into from
Jun 27, 2024

Conversation

TalDerei
Copy link
Contributor

@TalDerei TalDerei commented Jun 22, 2024

references #59

watch-and-repack.sh script simulates hot module replacement (HMR) by using fswatch to detect changes in the penumbra-zone/web repository. I think the script can be configured to optionally use the watch capabilities of tsc. When changes are detected, the script performs the following actions in sequence:

1. triggers pack-public.sh and repacks the packages.
2. installs the tarballs as dependencies in prax.
3. reloads webpack.

this process was tricky to set up because fswatch was initially continuously triggering the repack process because the repack itself was causing changes that fswatch picked up, leading to an infinite loop.


to use the prax dev script

(0) enable script permissions in penumbra-zone/web with chmod +x pack-public.sh and in prax-wallet/web withchmod +x watch-and-repack.sh.

(1) set the environment variables PENUMBRA_ZONE_WEB_PATH and PRAX_REPO_PATH with relative paths.

(2) remove npm dependencies from prax-wallet/apps/extension/package.json and any other local packages, e.g.

    "@penumbra-zone/bech32m": "^6.1.0",
    "@penumbra-zone/client": "^8.0.0",
    "@penumbra-zone/crypto-web": "^5.0.0",
    "@penumbra-zone/getters": "^8.0.0",
    "@penumbra-zone/keys": "^4.1.0",
    "@penumbra-zone/perspective": "^6.0.0",
    "@penumbra-zone/protobuf": "^5.1.0",
    "@penumbra-zone/query": "^6.0.0",
    "@penumbra-zone/services": "^6.0.0",
    "@penumbra-zone/storage": "^6.0.0",
    "@penumbra-zone/transport-chrome": "^4.0.0",
    "@penumbra-zone/transport-dom": "^7.1.0",
    "@penumbra-zone/types": "^9.0.0",
    "@penumbra-zone/wasm": "^9.0.0",

(3) execute pnpm install.

(4) execute pnpm run watch-and-repack, and it will wait for changes in penumbra-zone/web before starting the execution process.


demo

hmr.mov

Copy link

changeset-bot bot commented Jun 22, 2024

⚠️ No Changeset found

Latest commit: fa7f0d3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

watch-and-repack.sh Outdated Show resolved Hide resolved
@TalDerei TalDerei marked this pull request as draft June 22, 2024 05:41
@TalDerei TalDerei marked this pull request as ready for review June 22, 2024 08:00
Copy link
Contributor

@turbocrime turbocrime left a comment

Choose a reason for hiding this comment

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

some negative things here. we should pair on this

watch-and-repack.sh Outdated Show resolved Hide resolved
watch-and-repack.sh Outdated Show resolved Hide resolved
watch-and-repack.sh Show resolved Hide resolved

# Reload webpack
reload_webpack() {
(cd "$PRAX_REPO_PATH" && pkill -f "webpack-dev-server")
Copy link
Contributor

Choose a reason for hiding this comment

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

pkill is not directory constrained. in scripts you should only kill by known pid or job control.

it should be possible to signal webpack to reload without killing it

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 originally was trying to reload webpack, but changes only seemed to propagate when webpack was killed and then reloaded with pnpm dev

Copy link
Contributor Author

@TalDerei TalDerei Jun 23, 2024

Choose a reason for hiding this comment

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

pkill uses pattern matching under the hood, so not being directory constrained isn't really relevant here. It's true that it will terminate any process matching the pattern regardless of the current directory, so if there are multiple webpack dev server instances running, pkill -f "pnpm run dev" will terminate all of them.

I used a unique pattern,webpack-prax-dev-server which only terminates webpack in prax.

Copy link
Contributor Author

@TalDerei TalDerei Jun 24, 2024

Choose a reason for hiding this comment

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

Screenshot 2024-06-24 at 3 00 43 PM

will work on the webpack because it currently seems to spawn a new instance for every refresh, consuming my system memory?

Copy link
Contributor

Choose a reason for hiding this comment

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

webpack should have a way to output a pid, or a way to instruct it to reload modules. and if that isn't feasible it should be wrapped in something that can. pkill is the wrong approach

Copy link
Contributor Author

@TalDerei TalDerei Jun 25, 2024

Choose a reason for hiding this comment

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

webpack has a built-in hot module replacement dev feature (https://webpack.js.org/guides/hot-module-replacement/), and we may be able to configure webpack.config.ts to use HMR, but it seems tricky.

I instead modified the relevant function to use kill over pkill for a specific pid running on a specified port, which prevents spawning the extraneous webpack processes above.

reload_webpack() {
  # Find the PID of the actively running webpack process 
  WEBPACK_PID=$(lsof -t -i:5175) 

  if [ -n "$WEBPACK_PID" ]; then
    kill -9 $WEBPACK_PID
  fi
  
  (cd "$PRAX_REPO_PATH" && pnpm run dev &)
}

@TalDerei TalDerei requested review from a team and turbocrime June 23, 2024 02:57
grod220
grod220 previously approved these changes Jun 25, 2024
@grod220 grod220 dismissed their stale review June 25, 2024 12:58

Actually, sorry see this is still in progress, my bad

@TalDerei TalDerei requested a review from a team June 27, 2024 08:13
Copy link
Contributor

@turbocrime turbocrime left a comment

Choose a reason for hiding this comment

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

still think there is a better way to acquire webpack pid but approving

@TalDerei TalDerei merged commit 4480b31 into main Jun 27, 2024
3 checks passed
@TalDerei TalDerei deleted the prax-hmr branch June 27, 2024 18:17
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.

3 participants