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

🚀 Feature Request: Pages functions with _worker.js does not hot reload for any changes other than the _worker.js file #4824

Closed
treeder opened this issue Jan 23, 2024 · 16 comments · Fixed by #6150
Assignees
Labels
enhancement New feature or request pages Relating to Pages pages-dev Relating to `pages dev` command

Comments

@treeder
Copy link

treeder commented Jan 23, 2024

Describe the solution

It would be nice for wrangler to reload on any changes in the directory and subdirectories. Currently it will only reload if _worker.js is changed. It won't reload if any of its dependencies are modified including files imported from the same directory.

Using:

npx wrangler pages dev ./public
@treeder treeder added the enhancement New feature or request label Jan 23, 2024
@lauri865
Copy link

lauri865 commented Feb 4, 2024

Agree. As a stop-gap fix, it'd also suffice to have a shortcut to reload everything in the terminal by pressing R for example.

Otherwise, the DX is incredibly poor if you edit anything but the handler files, as you either have to make random changes to the handler file to get it to reload or have to restart the dev server altogether, which takes quite some time.

@treeder
Copy link
Author

treeder commented Mar 4, 2024

@lrapoport-cf any reason you removed this from the workers-sdk project?

@admah
Copy link
Contributor

admah commented Mar 27, 2024

@treeder the workers-sdk project is for untriaged issues and bugs. Liz move this to our roadmap project since it is something we intend to address in the future. All feature requests go there in order for us to prioritize them.

@treeder
Copy link
Author

treeder commented Mar 28, 2024

@lauri865 did you figure out some workaround for this? You're right, the dev experience is really bad due to this. If wrangler started up faster or something, it may be less of a pain, but everything is just too slow.

@CarmenPopoviciu CarmenPopoviciu added pages Relating to Pages pages-dev Relating to `pages dev` command labels Apr 24, 2024
@CarmenPopoviciu CarmenPopoviciu changed the title Pages functions with _worker.js does not hot reload for any changes other than the _worker.js file 🚀 Feature Request: Pages functions with _worker.js does not hot reload for any changes other than the _worker.js file Apr 24, 2024
@CarmenPopoviciu
Copy link
Contributor

triaging this as part of https://github.com/cloudflare/workers-sdk/milestone/10 to discuss with the team if this is something we want to take on as part of that body of work.

@CarmenPopoviciu
Copy link
Contributor

hi @treeder, just want to confirm...are you referring to changes in the _worker.js dependencies only, or are you referring to changes in the static assets as well?

@treeder
Copy link
Author

treeder commented Apr 29, 2024

Anything. It doesn't hot reload for anything except _worker.js. It SHOULD reload for everything for it to be useful including static assets and dependencies.

@treeder
Copy link
Author

treeder commented May 21, 2024

This seems like a really simple fix since you already have it for some directories, but a HUGE developer QoL win.

Can it be fast tracked?

@CarmenPopoviciu CarmenPopoviciu self-assigned this Jun 6, 2024
@CarmenPopoviciu
Copy link
Contributor

Hi folks

Picking this up as part of https://github.com/cloudflare/workers-sdk/milestone/10. Apologies for the wait 🙏 .

I will circle back here with more details, as I first have to look more thoroughly into why this is happening. Until then though, a few quick notes on my side:

  • the two main issues at hand seem to be that we're not reloading on 1. static assets changes, and 2. dependencies changes. Turns out, the mechanisms through which we would handle these two use cases are different atm, and do not live in the same code path, and ideally should be handled separately.
  • reloading on static assets changes currently requires users to pass the --live-reload flag to wrangler pages dev (we added support for it here granted it never seems to have worked properly? :sad-panda:). AFAIU, Pages support for pages dev --live-reload was meant to keep the dev experience in line with wrangler dev, though I personally question a bit how much that flag makes sense for Pages. I mainly wonder if there is ever a situation when Pages users would not want to reload on static assets changes in dev mode 🤔. I don't have an answer to this, and will have to discuss with product, but I am curious if anyone here has opinions on the matter they'd like to share?
  • reloading on dependency changes actually used to work, but we seem to have introduced a regression with Fix: Avoid unnecessary rebuilding pages functions in wrangler pages dev #3311. I need to double check this, but preliminary testing would seem to indicate that. I don't have enough context on those changes to know whether the regression was intentional or not, and I do not know if re-enabling esbuild's watch mode for Pages would not introduce the same performance issues as those reported in 🐛 BUG: Build cascading with pages dev #3258. Something I need to look into. But I agree with the general sentiment that this is something we should support

Moving forward, I think it makes sense to split this work up into two main threads:

  1. reload on static assets changes, which I will track in 🐛 BUG: wrangler pages dev --live-reload does not watch static assets #5351. This work is potentially blocked on some ongoing StartDevWorker improvements effort currently being carried out by the wrangler team, and might have to wait until that effort is completed.
  2. reload on deps changes, which I will track in this issue, and will start working on as of today 🎉

As I mentioned before, I will follow up here with more details as I am looking into this issue, but please feel free to drop any thoughts on anything I have mentioned so far. I abs empathise with the general feeling here that improving the pages dev experience would be such a HUGE win for everyone, which is why I would so much like us to get this right!

<3

@treeder
Copy link
Author

treeder commented Jun 6, 2024

For what it's worth, I think it should just watch for any change in my directory and subdirectories, minus probably node_modules and things like that.

It seems to currently only watch _worker.js and the functions directory.

I don't think watching external dependencies is necessary if that's what you're saying 2 is about, those changes are rare and would most likely require you to stop and run some npm commands anyways. I'm sure there's some kind of performance hit for watching some huge directory like node_modules too.

@CarmenPopoviciu
Copy link
Contributor

by "directory" do you mean your project's root, from where you are running the pages dev command, or the directory you pass to the command pages dev <DIRECTORY>?

And indeed, I wasn't thinking about watching things like node_modules as that would probably be expensive. By external dependencies I was more meaning user code that gets imported inside of Pages Functions, that lives somewhere in the project, but not necessarily inside the /functions folder. So I think we are talking about the same things

@treeder
Copy link
Author

treeder commented Jun 6, 2024

By directory, I suppose just the <DIRECTORY> passed to the command is what's needed.

And yes, sounds like the same thing.

CarmenPopoviciu added a commit that referenced this issue Jun 26, 2024
The watch mode in `pages dev` for Advanced Mode projects
is currently partially broken, as it only watches for
changes in the `_worker.js` file, but not for changes in
any of its imported dependencies. This means that given
the following `_worker.js` file

```
import { graham } from "./graham-the-dog";
export default {
    fetch(request, env) {
	return new Response(graham)
    }
}
```

`pages dev` will reload for any changes in the `_worker.js`
file itself, but not for any changes in `graham-the-dog.js`,
which is its dependency.

Similarly, `pages dev` will not reload for any changes in
non-JS module imports, such as wasm/html/binary module
imports.

This commit addresses all the aforementioned issues.

Fixes #4824
CarmenPopoviciu added a commit that referenced this issue Jul 2, 2024
The watch mode in `pages dev` for Advanced Mode projects
is currently partially broken, as it only watches for
changes in the `_worker.js` file, but not for changes in
any of its imported dependencies. This means that given
the following `_worker.js` file

```
import { graham } from "./graham-the-dog";
export default {
    fetch(request, env) {
	return new Response(graham)
    }
}
```

`pages dev` will reload for any changes in the `_worker.js`
file itself, but not for any changes in `graham-the-dog.js`,
which is its dependency.

Similarly, `pages dev` will not reload for any changes in
non-JS module imports, such as wasm/html/binary module
imports.

This commit addresses all the aforementioned issues.

Fixes #4824
CarmenPopoviciu added a commit that referenced this issue Jul 3, 2024
The watch mode in `pages dev` for Advanced Mode projects
is currently partially broken, as it only watches for
changes in the `_worker.js` file, but not for changes in
any of its imported dependencies. This means that given
the following `_worker.js` file

```
import { graham } from "./graham-the-dog";
export default {
    fetch(request, env) {
	return new Response(graham)
    }
}
```

`pages dev` will reload for any changes in the `_worker.js`
file itself, but not for any changes in `graham-the-dog.js`,
which is its dependency.

Similarly, `pages dev` will not reload for any changes in
non-JS module imports, such as wasm/html/binary module
imports.

This commit addresses all the aforementioned issues.

Fixes #4824
CarmenPopoviciu added a commit that referenced this issue Jul 4, 2024
The watch mode in `pages dev` for Advanced Mode projects
is currently partially broken, as it only watches for
changes in the `_worker.js` file, but not for changes in
any of its imported dependencies. This means that given
the following `_worker.js` file

```
import { graham } from "./graham-the-dog";
export default {
    fetch(request, env) {
	return new Response(graham)
    }
}
```

`pages dev` will reload for any changes in the `_worker.js`
file itself, but not for any changes in `graham-the-dog.js`,
which is its dependency.

Similarly, `pages dev` will not reload for any changes in
non-JS module imports, such as wasm/html/binary module
imports.

This commit addresses all the aforementioned issues.

Fixes #4824
CarmenPopoviciu added a commit that referenced this issue Jul 4, 2024
The watch mode in `pages dev` for Advanced Mode projects
is currently partially broken, as it only watches for
changes in the `_worker.js` file, but not for changes in
any of its imported dependencies. This means that given
the following `_worker.js` file

```
import { graham } from "./graham-the-dog";
export default {
    fetch(request, env) {
	return new Response(graham)
    }
}
```

`pages dev` will reload for any changes in the `_worker.js`
file itself, but not for any changes in `graham-the-dog.js`,
which is its dependency.

Similarly, `pages dev` will not reload for any changes in
non-JS module imports, such as wasm/html/binary module
imports.

This commit addresses all the aforementioned issues.

Fixes #4824
@CarmenPopoviciu
Copy link
Contributor

@treeder we merged a fix for this yesterday. Please give it a spin and let us know if you have any feedback <3

@treeder
Copy link
Author

treeder commented Jul 5, 2024

Awesome, thanks, we'll to try it out.

@treeder
Copy link
Author

treeder commented Jul 8, 2024

Seems to work! Thank you.

@CarmenPopoviciu
Copy link
Contributor

really happy to hear! thank you for reporting the issue to us <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pages Relating to Pages pages-dev Relating to `pages dev` command
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants