Skip to content

Use Deno cache to hydrate the deno dependencies, so they're pre-cached #136

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Use Deno cache to hydrate the deno dependencies, so they're pre-cached #136

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6eb5cfc
Use Deno cache to hydrate the deno dependencies, so they're pre-cache…
hicksy May 27, 2021
66bebb6
various improvements linked to CR
hicksy May 31, 2021
1eca862
Merge branch 'master' into hydrate-deno
hicksy Jul 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/actions/install-update.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
let { dirname, join, sep } = require('path')
let { dirname, join, sep, basename } = require('path')
let { existsSync } = require('fs')
let child = require('child_process')
let series = require('run-series')
Expand Down Expand Up @@ -34,6 +34,7 @@ module.exports = function hydrator (params, callback) {
let isJs = file.endsWith('package.json')
let isPy = file.endsWith('requirements.txt')
let isRb = file.endsWith('Gemfile')
let isDeno = file.endsWith('deps.ts') || file.endsWith('index.js') || file.endsWith('index.ts') || file.endsWith('index.tsx') || file.endsWith('mod.js') || file.endsWith('mod.ts') || file.endsWith('mod.tsx')
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we can tidy this up with an array + a some() check

Copy link
Author

Choose a reason for hiding this comment

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

Yep, cool. No probs. Will push some amends later


series([
function clear (callback) {
Expand All @@ -43,7 +44,9 @@ module.exports = function hydrator (params, callback) {
if (isJs) dir = join(cwd, 'node_modules')
if (isPy) dir = join(cwd, 'vendor')
if (isRb) dir = join(cwd, 'vendor', 'bundle')
rm(dir, callback)
if (isDeno) callback()
Copy link
Member

Choose a reason for hiding this comment

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

Why would we skip Deno? We definitely need a fresh function when hydrating, we don't know when the last time dependencies were pulled or what state they're in, this prevents a lot of transient dependency issues (and enables Sandbox symlinking).

Copy link
Author

@hicksy hicksy May 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe I've misunderstood - I thought the rm of those dirs was because for the other runtimes the shared & view folders are symlinked into the dependency folder? So like in node you can
let sharedFunc = require('@architect/shared/function.js)

I was thinking because in Deno vendor is just a standard dir it wouldn't need cleaning up?

else
rm(dir, callback)
}
else callback()
},
Expand Down Expand Up @@ -103,6 +106,12 @@ module.exports = function hydrator (params, callback) {
exec(`bundle update`, options, callback)
}

// cache deno deps.ts
else if (isDeno) {
// should --reload be added? That would force re-caching everytime, so maybe not?
Copy link
Member

Choose a reason for hiding this comment

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

Hydrate is responsible for deterministic deploys, prob not a bad idea to use --reload but I'm not sure what the re-caching operations are in Deno-land – can you speak to that at all?

Copy link
Author

@hicksy hicksy May 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

A little, so my understanding of Deno's cache is that any remote URL that's imported is cached, and the cache is stored as a reference to the URL. A file with a filename hash (of the pathname I think) stored in $DENO_DIR/deps/<http|s>/<hostname>

So (I think) it works like this:

  1. is the import a remote URL. If so, it'll be downloaded and cached (unless it's already cached, in which case its imported from the cache location)

If the --reload flag is supplied to either deno cache or deno run then regardless of whether the file is in the cache already, the remote URL will be downloaded and cached prior to import

  1. If it's a local import, that doesn't get cached.

I guess the downside of always using --reload will be the wasteful nature of downloading the same files over and over (slightly unavoidable on the first sandbox start as we're using discreet DENO_DIR local to the lambda).

But the downside to deno cache is it works great when your URL is pinned to a specific version, but if it's a generic like https://esm.sh/react-helmet then that URL will be cached even though over time that may be serving v2, v3, etc

So maybe it is best to take the hit, at least pre-caching like this keeps the invocations nice and fast

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, damn, bummer about the import-URLs-with-no-version. With that in mind I guess it makes sense to --reload all the time. As was said, we take the performance hit up-front at author time during hydrate (and thus before a deploy or sandbox run) in order to keep runtime performant.

Copy link
Author

Choose a reason for hiding this comment

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

OK, sure. Latest commit takes that approach

Copy link
Author

Choose a reason for hiding this comment

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

You'll see Deno's cache output in stdout, can add --quiet to the cache command to prevent that

Also, as we're caching the lambda handler, this doesn't quite follow the format of the logging

✓ Hydrate Successfully hydrated dependencies
⚬ Hydrate Hydrating dependencies in 1 path
✓ Hydrate Hydrated frontend/home//home/

Hydrate Hydrated frontend/home//home/ should really be Hydrate Hydrated frontend/home/index.tsx

Think that uses dirname somewhere?

exec(`DENO_DIR=./vendor/.deno_cache deno cache --unstable ./${basename(file)}`, options, callback)
}

else {
callback()
}
Expand Down
25 changes: 25 additions & 0 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
let { sync: glob } = require('glob')
let series = require('run-series')
let { dirname, join } = require('path')
let { existsSync: exists } = require('fs')
let stripAnsi = require('strip-ansi')
let { pathToUnix, updater } = require('@architect/utils')
let inventory = require('@architect/inventory')
Expand Down Expand Up @@ -69,6 +70,30 @@ function hydrator (inventory, installing, params, callback) {
if (viewsDir && file.includes(viewsDir)) return false
return true
})

// Add deno cacheable files if they exist and we're hyrdrating a Deno runtime
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this change would be the right approach. How about adding these to the glob pattern, and then filter for Deno by entry files on Deno Lambdas?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, cool. No probs. Will push some amends later

Copy link
Author

Choose a reason for hiding this comment

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

Does the latest commit work better?

let lambda = inv.lambdasBySrcDir[basepath]
if (lambda !== undefined && lambda.config !== undefined) {
let isDeno = lambda.config.runtime === 'deno'
if (isDeno) {
let denoCacheableFiles = [
'index.js',
'mod.js',
'index.ts',
'mod.ts',
'index.tsx',
'mod.tsx',
'deps.ts'
]
denoCacheableFiles.map(denoFile => {
let file = join(basepath, denoFile)
if (exists(file)) {
files.push(file)
}
})
}
}

// Get shared + views (or skip if hydrating a single isolated function, e.g. sandbox startup)
if (hydrateShared) {
let sharedManifest = (sharedDir && glob(pattern(sharedDir)).filter(ignoreDeps)) || []
Expand Down
2 changes: 1 addition & 1 deletion src/lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
let { pathToUnix } = require('@architect/utils')
let isDep = file => file.includes('node_modules') || file.includes('vendor/bundle')
let isDep = file => file.includes('node_modules') || file.includes('vendor/bundle') || file.includes('.deno_cache')
let ignoreDeps = file => !isDep(pathToUnix(file))

// Relativize by stripping leading relative path + `.`, `/`, `./`, `\`, `.\`
Expand Down