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

feat: add support for denoDir parameter #80

Merged
merged 2 commits into from
Jul 29, 2022
Merged

Conversation

eduardoboucas
Copy link
Member

Which problem is this pull request solving?

Adds a denoDir parameter to the bundle function. When set, its value is used to set the DENO_DIR environment variable.

List other issues or pull requests related to this problem

https://github.com/netlify/pod-compute/issues/161

@eduardoboucas eduardoboucas added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Jul 28, 2022
@eduardoboucas eduardoboucas requested a review from a team July 28, 2022 16:24
}

if (cacheDirectory !== undefined && featureFlags.edge_functions_cache_deno_dir) {
options.denoDir = join(cacheDirectory, 'deno_dir')
Copy link
Contributor

Choose a reason for hiding this comment

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

cacheDirectory is set by build to a cachable directory?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. See here and here.

Copy link
Contributor

@danez danez Jul 28, 2022

Choose a reason for hiding this comment

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

Could that potentially mean that dependencies which are cached to never get updated? or is the full version always included in the URL?
Kind of unrelated:
Same actually for the cli, if we cache it in buildbot it will never get updated? Also why not install the deno CLI in the build-image directly, to save time and traffic on caching and downloading/installing deno? Sorry for these questions just trying to understand the history.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could that potentially mean that dependencies which are cached to never get updated? or is the full version always included in the URL?

Yes, the version should be included in the URL. Having a permanent directory that the Deno CLI can use to cache different things is its normal operation mode. This means not only caching remote dependencies, but also the result of compiling TypeScript to JavaScript.

Currently, we're forcing it to "start from scratch" on every build, which comes with a performance penalty.

Same actually for the cli, if we cache it in buildbot it will never get updated?

We store the version of the CLI that is cached locally and we compare it against the version that Edge Bundler requires. If there's not a match, we download a fresh version. You can see that logic here.

Also why not install the deno CLI in the build-image directly, to save time and traffic on caching and downloading/installing deno?

See https://github.com/netlify/pillar-workflow/issues/504.

Sorry for these questions just trying to understand the history.

No problem at all. These are great questions.

Copy link
Contributor

@danez danez left a comment

Choose a reason for hiding this comment

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

Lgtm

@eduardoboucas eduardoboucas merged commit b5dd4a7 into main Jul 29, 2022
Skn0tt pushed a commit to netlify/build that referenced this pull request Apr 23, 2024
* feat: add support for `denoDir` parameter

* chore: improve test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants