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

Improved deno run --watch support, watch support inside Docker #79

Merged
merged 13 commits into from
Nov 30, 2023

Conversation

daniel-chambers
Copy link
Contributor

@daniel-chambers daniel-chambers commented Nov 28, 2023

Description

This PR improves support for running the connector using deno run --watch where the connector gets restarted by Deno whenever the function files change. It adds support for running the connector in watch mode using Docker.

docker build . -t "typescript-deno:latest" # only because we don't publish a docker container anywhere
docker run -it --rm -p 8080:8080 -e WATCH=1 -v ./functions:/functions/src typescript-deno:latest

Part of the improvements to make watch mode work better include

  • rearranging when we infer the functions schema to reduce work done, but also reduce watch-induced reloads caused by vendoring during inference
  • rearranging the dockerfile and entrypoint script so that the generated vendor, node_modules and schema.json artifacts are stored outside of the functions source directory, so that we don't pollute the user's volume mount

Various version updates have also been made:

  • Version of deno used in Docker has been updated
  • Deno github action has been updated
  • the deno std library version has been updated and made consistent across the entire project

Solution Design

We now have a difference between RawConfiguration and Configuration. We perform our inference only in validate_raw_configuration and capture the results in the Configuration. This then powers every usage of the inference, including the schema endpoint and actual queries. It also provides a central place for configuration defaulting (such as defaulting to "INFER").

The entrypoint script now looks for a WATCH env var and if it is "1" or "true" it generates a different configuration that enables INFER mode and turns on deno's --watch flag.


Fixes: #26

@daniel-chambers daniel-chambers self-assigned this Nov 28, 2023
@sordina
Copy link
Contributor

sordina commented Nov 28, 2023

Things to test:

  • CI published docker images
  • Use of these images from Sendgrid example project
  • Connector create workflow
  • NPM Deps
  • Deno Deps
  • Local file deps
  • Watch reload triggering
  • Direct use (Test after tag is made available)
  • docker use
  • Infer command

It would be good if we can introduce some automated / e2e CI testing for these categories at some point - extending the current tests beyond inference. But we can just manually test for this PR.

Copy link
Contributor

@sordina sordina left a comment

Choose a reason for hiding this comment

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

Awesome work and great cleanup with the validate phase.

I'd say the only change I'd like is to split the entrypoint script, since we're adding more logic into here with watch.

Also have a few comments and questions inline.

@@ -123,7 +123,6 @@ Limitations:
* All numbers are exported as `Float`s
* Unrecognised types will become opaque scalars, for example: union types.
* Optional object fields are not currently supported
* Complex input types are supported by the connector, but are not supported in "commands" in Hasura3 projects
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

Comment on lines -152 to -162
* (Optionally) If you want your development vendor and inference resources to be used to speed up deployment, add the following to your `./config.json`:
```json
{
"functions": "./functions/index.ts",
"vendor": "./functions/vendor",
"preVendor": true,
"schemaLocation": "./functions/schema.json",
"schemaMode": "INFER"
}
```
* Make sure to .gitignore your computed `vendor` and `schema.json` files.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines -175 to +163
functions (string): Location of your functions entrypoint (default: ./functions/index.ts)
functions (string): Location of your functions entrypoint
Copy link
Contributor

Choose a reason for hiding this comment

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

Why no documented default for functions, but present for preVendor, schemaMode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't default functions any more, you must specify it as something yourself, so there is no default. You will get an error if you leave it out or put in a blank string. In all the places where we control the locations of the functions (ie. in docker) we specify it. We specify it in all the documentation. We also generate a value in a blank configuration object.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for this? I thought it was an advantage minimise boilerplate in the config.

const defaults = {
preVendor: true,
validate_raw_configuration(configuration: RawConfiguration): Promise<Configuration> {
if (configuration.functions.trim() === "") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an odd check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't leave out functions any more. You must specify it.


# Pre-cache inference results and dependencies
RUN EARLY_ENTRYPOINT_EXIT=true sh /app/entrypoint.sh
RUN PRECACHE_ONLY=true /app/entrypoint.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably just split the initial actions into a seperate script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What, and call it from this script? It still needs to happen on container start for those volume-mounting their functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Call the build actions from the dockerfile directly or via a build script, then remove them from the entrypoint script

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right... yeah this would work on connector create, but not for people doing local docker... bummer

Changes to be included in the next upcoming releaase.
Changes to be included in the next upcoming release.
Copy link
Contributor

Choose a reason for hiding this comment

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

Doy.

@sordina
Copy link
Contributor

sordina commented Nov 29, 2023

Should probably have a better error if the config is missing the functions:

> deno run -A --watch --check ~/hasura/ndc-typescript-deno/src/mod.ts serve --configuration config.json
Watcher Process started.
Check file:///Users/lyndon/hasura/ndc-typescript-deno/src/mod.ts
TypeError: Cannot read properties of undefined (reading 'trim')

@daniel-chambers
Copy link
Contributor Author

Weird. I thought the SDK would prevent it from getting that far in the code :/ The types are a lie.

@sordina
Copy link
Contributor

sordina commented Nov 29, 2023

Fatal errors seem to kill the watch command in some instances:

Watcher File change detected! Restarting!
Inferring schema with map location /Users/lyndon/temp/temp-2023-11-29_14-33-24-rtYNSYAuhX/vendor
Vendoring dependencies: /Users/lyndon/bin/binaries/deno vendor --node-modules-dir --output /Users/lyndon/temp/temp-2023-11-29_14-33-24-rtYNSYAuhX/vendor --force /Users/lyndon/temp/temp-2023-11-29_14-33-24-rtYNSYAuhX/functions/index.ts
Couldn't find import map: /Users/lyndon/temp/temp-2023-11-29_14-33-24-rtYNSYAuhX/vendor/import_map.json
There were 1 diagnostic errors.
FATAL: /Users/lyndon/temp/temp-2023-11-29_14-33-24-rtYNSYAuhX/functions/index.ts (10,30): The return type of an async function or method must be the global Promise<T> type. Did you mean to write 'Promise<string>'?
Fatal errors: 1
Previous comand exited with error status: 1

@sordina
Copy link
Contributor

sordina commented Nov 29, 2023

There's not much point having the two versions:

  • programInfo
  • programInfoException

after making programInfo just propagate exceptions rather than exit.

Copy link
Contributor

@sordina sordina left a comment

Choose a reason for hiding this comment

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

Let's goooo!

Follow up items:

  • test direct invocation off deno.com
  • non-null coalescense
  • test Docker image from registry
  • test with new CLI tools
  • Tag new release
  • Add changelog entry for new tag
  • Notify channels of new release

@daniel-chambers daniel-chambers merged commit a686cf7 into main Nov 30, 2023
1 check passed
@daniel-chambers daniel-chambers deleted the daniel/deno-run-watch branch November 30, 2023 01:19
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.

Support nested field selection [+Engine]
2 participants