sst icon indicating copy to clipboard operation
sst copied to clipboard

[RFC] Hot reload (Live Lambda Development) doesn't work if function is not in subfolder

Open zvictor opened this issue 3 years ago • 20 comments

Description

SST only watches for functions that are located in one of the subfolders of an SST project. As consequence, it makes it hard to be integrated into an already existing monorepo.

For instance, SST does not allow for hot reloading in a project that has this structure:

.
├── deployment/                 <-- The SST project
    ├── stacks/
    │   ...
    └── sst.config.ts
└── functions/                  <-- The folder with the lambda projects
    ├── lambda1/
    │   ├── index.ts
    │   └── package.json
    ├── lambda2/
    │   ├── index.ts
    │   └── package.json

Reproduction

  1. Run the cmds below
$ cd /tmp
$ npm init sst
$ mv my-sst-app/packages packages
$ (cd packages/core ; npm i)
$ (cd packages/functions ; npm i)
$ cd my-sst-app ; npm install ; npm run dev
  1. Replace "./packages/functions/src/lambda.handler" by "../packages/functions/src/lambda.handler" at my-sst-app/stacks/MyStack.ts:6
  2. Edit the functions code (e.g. add console.log('hello') inside /tmp/packages/functions/src/lambda.ts) and observe the results. Only the first run will be correct, while no subsequent invocations will have the code updated.

Notes

  • I have tried it out on SST versions 2.1.12, and 2.1.4
  • I tried symlinking the functions folder inside the SST project, but it didn't help

zvictor avatar Mar 07 '23 16:03 zvictor

Okay, I have looked deeper into SST's code and there are a few assumptions I think I can make from my findings:

A. There is a centralised watcher which is being called in 2 different places. The logic couldn't be simpler (👏): https://github.com/serverless-stack/sst/blob/186b545b3795f6a285534ff05d196ab8a04e3daa/packages/sst/src/watcher.ts#L20-L23 B. Not allowing for symlinks as well as watching only the insides of the SST project seem to be "by design / intentional". C. There is currently no workaround possible for the issue being reported.

Thus, it seems safe to conclude that SST is not friendly with monorepos structured differently than in the only way accepted by SST (which is the case for pretty much any pre-existing project 🤷‍♀️).

zvictor avatar Mar 07 '23 23:03 zvictor

Here are my 2 proposals:

Solution A

Whenever a Function is registered and it's located outside project.paths.root, add the handler location to the list of locations being watched. E.g.:

 const watcher = chokidar.watch([project.paths.root, ...foreignHandlerLocations], { 
   ...

The biggest challenge with this design is that the handler always points to a single file, while the user might be interested in reacting to changes in sibling and parent files and folders as well. Would it be enough to just watch the handler file - or its parent folder - of each registered function?

Solution B

Add an option to the cli in order to give the control to the user. E.g.:

sst dev --watch ../function/folder --watch ../../package/folder
sst dev --watch ../function/folder,../../package/folder

This solution is more powerful, but comes with the cost of extra complexity to the CLI.


Please share your thoughts! I am capable of implementing solution #B, but would need help with #A.

zvictor avatar Mar 08 '23 00:03 zvictor

do you think the watcher looking for a parent git folder could make sense?

thdxr avatar Mar 08 '23 00:03 thdxr

do you think the watcher looking for a parent git folder could make sense?

Interesting idea! I think that in my specific case, yes. All my files are still inside the same git repository, so that should work well!

But being the devils' advocate here: this solution won't fit everyone.

When I first experienced the reported issue, I was disappointed that SST allowed me to register a function - without any warnings - but only provided "partial support" to it (no hot reload). There was a silent error to be discovered somewhere, and I would have been happier with an explicit error instead. 🙂

My question back to you is:

  • What should happen when someone at ~/my-project (git project) inadvertently tries to register a handler at /tmp/my-silly.handler?
    • Should we throw an Invalid handler location. Stay within git boundaries!? 😅
    • Or should we still support a --watch option for these very edgy cases?

zvictor avatar Mar 08 '23 00:03 zvictor

All in all, I think we could simply:

  1. Set the watcher to look for the closest parent version control system folder (.git, .hg, .svn, etc). 1.1. If no such folder can be found, default to the sst root folder as it is today.
  2. Whenever registering a Function, check if the handler is within the watcher boundaries. And if not, warn the user.

Doing so would cover the absolute majority of use cases, I hope. It would also leave the possibility open for future users to request the --watch option in case they can present use cases strong enough to justify the added complexity cost.

zvictor avatar Mar 08 '23 01:03 zvictor

@zvictor just a quick question (maybe you answered it already), but why not put the sst.config.ts in the root? You can leave the stacks code elsewhere.

jayair avatar Mar 09 '23 20:03 jayair

+1 running in to this issue as well.

@jayair - from my perspective, I have multiple SST apps (monorepo setup) that would not work if sst.config.ts was at root level.

tim-lar avatar Mar 23 '23 05:03 tim-lar

@tim-lar can you describe your project structure?

jayair avatar Mar 24 '23 21:03 jayair

@tim-lar can you describe your project structure?

This issue also affects me - maybe my info can help. My app is within a monorepo and its module structure is dictated by rules laid out for all apps in the repo. (The other apps are not necessarily using SST.)

The structure is roughly:

[app-name]
├── infra
│   ├── package.json
│   ├── sst.config.ts
│   └── stacks
│       └── index.ts
└── workers
    ├── [worker-1]
    │   ├── package.json
    │   └── src
    │       └── index.ts
    └── [worker-2]
        ├── package.json
        └── src
            └── index.ts

So I run sst dev from within ./infra, and the worker source code is not picked up for hot-reloading.

tobz1000 avatar Apr 05 '23 13:04 tobz1000

@tobz1000 what stops you from moving only the sst.config.js one level up and update the imports accordingly? In my case that would actually work, besides the ugly aesthetics of having the main "infra" file outside of the "infra" folder, but that's acceptable. Is there anything else that stops you from making that move?


@tim-lar do you have multiple sst.config.js files in the same repo and therefore you can't have a single sst.config.js at the root. Is that correct? Can you tell us a bit more on how you set up your project?

zvictor avatar Apr 08 '23 15:04 zvictor

what stops you from moving only the sst.config.js one level up and update the imports accordingly?

This is what I've ended up doing. I actually have an sstConfig.ts within the infra project, and the sst.config.ts used by the binary in the parent dir simply imports/exports the content of sstConfig.ts.

This works fine, but relies on the sst binary being available at the correct version outside of any npm project directory.

tobz1000 avatar Apr 11 '23 03:04 tobz1000

For anyone else dealing with this, an addendum to the above solution: function hot-reloading works with sst.config.ts in a parent directory, but for hot-redeployment to work, the file also needs to be in the same directory as a package.json. (It took me a month to notice this...)

So my new dir structure is:

[app-name]
├── package.json
├── sst.config.ts
├── infra
│   ├── package.json
│   └── stacks
│       └── index.ts
└── workers
    ├── [worker-1]
    │   ├── package.json
    │   └── src
    │       └── index.ts
    └── [worker-2]
        ├── package.json
        └── src
            └── index.ts

The downside is that I now have nested package definitions in my project structure (workspace definitions are handled outside of the app entirely), which seems like bad form. But all hot-updating works now.

tobz1000 avatar May 06 '23 11:05 tobz1000

Is there an ETA on when this will be fixed? I'm about to start a new greenfield project and want to avoid a single huge repo containing everything. Since the CF stack eventually grows too big causing problems in AWS.

Anthon129 avatar Jun 03 '23 21:06 Anthon129

Hmm I don't think that's a good pattern. We typically recommend a mono repo.

jayair avatar Jun 04 '23 02:06 jayair

For anyone interested, I run this as part of my postinstall script:

(sed -i "" "s|chokidar.watch(\[project.paths.root\]|chokidar.watch([path.join(project.paths.root, '..')]|g" ./deployment/node_modules/sst/watcher.js) && echo "SST's watcher has been patched"

It patches sst inside ./deployment to check one folder up. It's a dirty-temporary-workaround.

zvictor avatar Jul 14 '23 18:07 zvictor

Oh that's interesting. Thanks for sharing.

jayair avatar Jul 14 '23 23:07 jayair

I was able to work around this issue until recently, when the commit https://github.com/sst/sst/commit/3becfd9458a7c2c2d86d0d77289307bbba5b8b6b by @fwang (released in [email protected]) changed the behaviour of sst bind.

If you depend on sst bind, you probably can't use the project structure discussed in this issue anymore. I have opened a discussion on that on Discord as well.

🗣️ How to use the "new" bind command?

zvictor avatar Oct 13 '23 14:10 zvictor

Yup replied

jayair avatar Oct 13 '23 18:10 jayair

Our Monorepo was setup with Serverless Framework, so it looks something like this:

apis/
├─ api-one/
│  ├─ handlers
│  ├─ serverless.yml
├─ api-two/
│  ├─ handlers
│  ├─ serverless.yml
packages/
├─ common-lib/
package.json

We like each API being self-contained, it makes it a lot easier to maintain and keep separate. Each API can depend on shared packages in ./packages so code is shared between them.

I tried setting up an SST project in the same manner and while having an sst.config.ts for each API does work hot reloading for shared packages doesn't.

Would be great to have a way to specifically tell SST to watch for changes in other folders.

Edit: Although after looking into it even after moving sst.config.ts to the root my Functions still aren't rebuilding even when a dependency is changes so not sure how this is supposed to work.

leo-petrucci avatar Apr 26 '24 11:04 leo-petrucci

This one has become problematic for us too. Any suggested solutions/workarounds/further thinking on this?

Our problem:

  • we have an sst.config.ts at the root, hot reloading works fine, but the the SST infra code has become an unmanagable mess
  • we were hoping to be able to refactor by creating a new sst app in app, that will reference the same function code in the same place as the legacy app
  • hot reloading no longer works
apis/
├─ api-one/
│  ├─ handlers
├─ api-two/
│  ├─ handlers
app/
├─ sst.config.ts  # New SST app that points to code in apis
sst.config.ts  # Legacy SST app that points to code in apis
package.json

dan-turner avatar Jun 11 '24 02:06 dan-turner