Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

[CLI] Watch generated projects #1234

Closed
virkt25 opened this issue Apr 4, 2018 · 16 comments
Closed

[CLI] Watch generated projects #1234

virkt25 opened this issue Apr 4, 2018 · 16 comments
Labels
CLI developer-experience Issues affecting ease of use and overall experience of LB users feature help wanted major needs discussion

Comments

@virkt25
Copy link
Contributor

virkt25 commented Apr 4, 2018

Description / Steps to reproduce / Feature proposal

Projects scaffolded and started using the CLI have a start script but any changes such as adding new controllers requires us to manually restart the server. This should be automated using tsc watch mode imo similar to how angular does it for it's projects.

I would propose to even go as far as creating a lb4 serve / lb4 start command similar to angular that can accomplish the watch mode for development purposes while npm start can be without watch mode for use in production.

Current Behavior

Restart the app manually when a new controller is added.

Expected Behavior

App restarts automatically once a new controller has been added to the project.

@dhmlau
Copy link
Member

dhmlau commented Apr 20, 2018

Closing in favor of #1259, since that issue has more detailed description.

@dhmlau dhmlau closed this as completed Apr 20, 2018
@virkt25
Copy link
Contributor Author

virkt25 commented Apr 20, 2018

This task is related but not the same ... I would say this is blocked by #1259 not a duplicate. This is a CLI improvement using the implementation of #1259

This is a proposal to add a lb4 start or lb4 serve command which starts the LB4 project with watch mode enabled (implemented in #1259).

@virkt25 virkt25 reopened this Apr 20, 2018
@virkt25
Copy link
Contributor Author

virkt25 commented Apr 20, 2018

If it was decided we don't want to implement this at all I'm ok to close it (with a message saying that).

@bajtos bajtos added the feature label Apr 23, 2018
@joeytwiddle
Copy link
Contributor

joeytwiddle commented Jun 28, 2018

This is how I do it used to do it: (For how I do it now, see below)

"start:watch": "nodemon -e 'js,ts' -w src -d 2 -x 'npm start || exit 1'",

"build:watch": "npm run clean && lb-tsc es2017 --outDir dist --watch",
"start:watch-light": "nodemon -e '*' -w dist -d 2 -x 'node dist/server.js || exit 1'",

The first one (start:watch) is slow, because npm start does a full rebuild.

The second approach uses the incremental compiler, but I do have to run two separate commands! (I use two terminal panes for that.) But it is certainly worthwhile: development goes much faster.

Occasionally the contents of dist will end up in a bad state. (I think this happens when a source file is removed: the corresponding file doesn't get removed from dist. If so, switching to and from older branches could trigger it.) That's why I added clean to the build:watch script. If something breaks I just restart build:watch to get a clean slate.

I also have a test:watch-light script which watches dist, and re-runs the tests whenever it changes.

It would be great to have all (three?) tasks in one command, but at least this setup is working well for us right now.

@virkt25
Copy link
Contributor Author

virkt25 commented Jun 28, 2018

@joeytwiddle Projects scaffolded using the lb4 CLI already have a build:watch script. Would you be interested in submitting a PR to add a start:watch to package.json scaffolded using the CLI since you have experience in this area?

@joeytwiddle
Copy link
Contributor

joeytwiddle commented Jul 1, 2018

Yes you are right, I only tweaked the existing build:watch very slightly.

I would be happy to do that.

But are you devs happy to add the extra nodemon dependency to the project?

Edit: Or perhaps tsc-watch would be preferable to nodemon. I haven't tried it yet.

@virkt25
Copy link
Contributor Author

virkt25 commented Jul 4, 2018

I think as long as the CLI scaffolding the app gives an option to have watch powered by either nodemon / tsc-watch it should be ok (similar to prettier / lb-build / tslint) BUT I would like to let others chime in on the issue to get their thoughts on adding a new dependency.

cc: @strongloop/sq-lb-apex @strongloop/loopback-next @raymondfeng @bajtos

@bajtos
Copy link
Member

bajtos commented Jul 9, 2018

I like that with tsc-watch, one needs only one terminal to see the output of both tsc and the application. The trouble is that tsc-watch does not work with our auto-selection of compilation target and dist folder depending on the current Node.js version (e.g. on Node.js 8.x, we use --target es2017 --outDir dist8).

To address this limitation, I am proposing to integrate tsc-watch into our lb-tsc helper:

  • lb-tsc - works as before (calls tsc directly)
  • lb-tsc --watch - works as before (calls tsc --watch directly)
  • lb-tsc --watch --onSuccess "node ." - calls tsc-watch under the hood

Alternatively, we can introduce a new command lb-tsc-watch which can share a lot of the implementation code with lb-tsc, but execute tsc-watch instead of tsc.

Either way, by exposing tsc-watch functionality through a helper script we control, it will be easier in the future to fix possible bugs of tsc-watch in case the maintainers of tsc-watch are not responsive.


Regarding nodemon: I believe our CLI tooling should not be offering unnecessary many options to our users - Decision Fatigue is a real thing. I personally prefer to offer LB users only one solution, the one which we consider as the best.

I don't have a strong opinion whether this solution should be nodemon, tsc-watch or something else.

Adding an option allowing users to opt-out of watch mode (and thus a dependency on nodemon) in a fashion similar to how they can opt-out of prettier/lb-build/tslint integration is fine with me.

@bajtos
Copy link
Member

bajtos commented Jul 9, 2018

FWIW, I think the implementation of tsc-watch could be simplified by using TypeScript Compiler API instead of calling out to tsc --watch script and parsing the script output. I am also not sure why tsc-watch executes system tools like taskkill and ps-tree to kill child processes (source) instead of using Node.js' process.kill() API - that's another source of extra dependencies.

What I am trying to say: if we decide to use tsc-watch and the additional dependency becomes a problem, then it should reasonably easy to refactor our wrapper to drop the dependency on tsc-watch and implement the functionality directly.

@bajtos
Copy link
Member

bajtos commented Nov 23, 2018

I like that with tsc-watch, one needs only one terminal to see the output of both tsc and the application. The trouble is that tsc-watch does not work with our auto-selection of compilation target and dist folder depending on the current Node.js version (e.g. on Node.js 8.x, we use --target es2017 --outDir dist8).

FWIW, auto-selection of compilation target & dist folder is no longer a problem, we have removed this feature while preparing for TypeScript Project References & Build mode - see #1803

@bajtos bajtos added developer-experience Issues affecting ease of use and overall experience of LB users help wanted CLI labels Nov 23, 2018
@stale
Copy link

stale bot commented Nov 18, 2019

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

@stale stale bot added the stale label Nov 18, 2019
@bajtos bajtos added major and removed stale labels Nov 28, 2019
@joeytwiddle
Copy link
Contributor

joeytwiddle commented Mar 26, 2020

The current release of loopback seems to use tsconfig.tsbuildinfo to cache previously compiled hashes, making update builds much faster than the old full rebuild.

As a result, I have stopped using a separate watcher, and I now do everything in one process:

    "prestart": "npm run build",

    // Loopback's bundled start script, which does not respond to changes
    "start": "node -r source-map-support/register .",

    // Use start:watch instead. After a change it will trigger prestart and start
    "start:watch": "nodemon -e '*' -w src -d 2 -x 'npm run start' || exit 1",

This takes a few seconds to update the build and restart the server, but I find the simplicity (only needing one terminal window) is worth the price.

I do the same with test:watch which is really useful for our developers to watch while they are working.

(Something similar might be possible with tsc --watch but I haven't tried it.)


I'm also not wedded to nodemon. I am just sharing a technique that I know works, for people who want something right now. If we can achieve the same functionality a tidier way, I'm all for it!

There are also a few alternatives to nodemon, for example three were described in this 2013 Strongloop blog post. 🙂

@dhmlau
Copy link
Member

dhmlau commented Apr 19, 2020

@bajtos had a proposal in #1234 (comment). @raymondfeng, any thoughts on which option to adopt to go forward? Thanks.

@dhmlau dhmlau added the 2020Q3 label Apr 19, 2020
@madaky
Copy link
Contributor

madaky commented May 18, 2020

@dhmlau, @bajtos : As per my observation the global trends says that most of the developer use nodemon for watch configuration of their projects, Also in this particular thread and other thread related to debug and watch most of the developer found easy integration with nodemon.
So far I also agree with @bajtos regarding own implementation for

lb-tsc --watch --onSuccess

in looback itself which may be a future feature with lbs' own watcher. we may put it for feature for future.

@dhmlau dhmlau removed the 2020Q3 label Jul 28, 2020
@bajtos
Copy link
Member

bajtos commented Sep 11, 2020

I like that with tsc-watch, one needs only one terminal to see the output of both tsc and the application. The trouble is that tsc-watch does not work with our auto-selection of compilation target and dist folder depending on the current Node.js version (e.g. on Node.js 8.x, we use --target es2017 --outDir dist8).

Please note the comment above is no longer true. At the moment, lb-tsc is very tiny wrapper around tsc, it mostly finds the right tsconfig.json file and automatically decides whether to use build mode (-b) for composite projects using project references.

@madaky I think tsc-watch should work in LoopBack projects out of the box now. No need to implement our own version.

You can also use the solution described by @joeytwiddle in #1234 (comment).

I guess the next step is to decide which solution we want to promote as "the right LoopBack way" and then update our CLI project templates to setup that solution for new projects, i.e. add "start:watch" script after "start":

https://github.com/strongloop/loopback-next/blob/6fbf23d3e6366a99ee0ff543fd05fe15bdb711ca/packages/cli/generators/project/templates/package.json.ejs#L99

@mbnoimi
Copy link

mbnoimi commented Dec 8, 2020

I guess the next step is to decide which solution we want to promote as "the right LoopBack way" and then update our CLI project templates to setup that solution for new projects, i.e. add "start:watch" script after "start":

Please take the decision; This issue opened in 2018!
I'm still learning LB4 and I already miss the feature. I was surprised why I can't execute npm run start:watch in LB4!

@bajtos bajtos closed this as completed Mar 11, 2021
@loopbackio loopbackio locked and limited conversation to collaborators Mar 11, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
CLI developer-experience Issues affecting ease of use and overall experience of LB users feature help wanted major needs discussion
Projects
None yet
Development

No branches or pull requests

6 participants