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

Updates docs when building a new command. Closes #5835 #5999

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SmitaNachan
Copy link
Contributor

Updates docs when building a new command. Closes #5835

@milanholemans
Copy link
Contributor

Thanks @SmitaNachan, we'l ltry to review it ASAP!

@Jwaegebaert Jwaegebaert self-assigned this Jul 17, 2024
@Jwaegebaert Jwaegebaert removed the docs label Jul 18, 2024
Copy link
Contributor

@Jwaegebaert Jwaegebaert left a comment

Choose a reason for hiding this comment

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

Hey @SmitaNachan, thanks for the doc updates, but we need to take a different direction. Like Waldek mentioned here: #5835 (comment), the goal is to upgrade the current watch command to start with a new build before attaching the watcher. Sorry for any confusion.

@Jwaegebaert Jwaegebaert marked this pull request as draft July 18, 2024 09:49
@SmitaNachan
Copy link
Contributor Author

SmitaNachan commented Jul 20, 2024

Hi @Jwaegebaert
Sorry for the confusion, but is it just a documentation level update (as per issue description) or we need to make changes to the command?
Please suggest.

@Jwaegebaert
Copy link
Contributor

@SmitaNachan, you shouldn't necessarily need to change any commands but rather make changes to the way that the npm run watch command works. Be sure to check out Waldeks comment on the issue: #5835 (comment)

@SmitaNachan
Copy link
Contributor Author

Hi @Jwaegebaert
Will it be a good idea to propose to run npm run build && npm run watch as a command?
This will help us build the command as needed first and then npm can watch for further changes. Please suggest.

@Jwaegebaert
Copy link
Contributor

@SmitaNachan, let's try some different approaches. Running npm run build && npm run watch isn't the most efficient, but we could run benchmark tests to get a clear picture.

I was thinking of exploring TypeScript CLI options more deeply. There's a section about the watcher with some interesting options we could use. https://www.typescriptlang.org/docs/handbook/compiler-options.html

Another idea is to focus on adding new commands during the watch session. Maybe we could include some of our pre-written scripts at the watch launch?

@SmitaNachan
Copy link
Contributor Author

SmitaNachan commented Jul 30, 2024

Hi @Jwaegebaert

I propose making below changes to the package.json:

"scripts": {
    "build": "tsc -p . && node scripts/write-all-commands.js && node scripts/copy-files.js",
    "watch": "tsc -w -p . && node scripts/write-all-commands.js && node scripts/copy-files.js",

Any opinions please?

@Jwaegebaert
Copy link
Contributor

That seems like a good start. Although I'm not quite sure if we'll need the copy-files.js, you'll need to see what the results are if you include or exclude it. Maybe for tsc, it could be handy that we look into some different options as well. There is a --force flag you can use that sounds interesting to use for the watch command.

@SmitaNachan
Copy link
Contributor Author

Hi @Jwaegebaert

Using --force option with watch gives below error:
error TS5093: Compiler option '--force' may only be used with '--build'

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.

Update docs when building a new command
3 participants