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: port snapshot_from_lockfile from denoland/deno #27

Merged
merged 12 commits into from
Aug 3, 2023

Conversation

magurotuna
Copy link
Member

@magurotuna magurotuna commented Aug 2, 2023

This commit brings snapshot_from_lockfile function to deno_npm from the main deno repository.

denoland/deno#20024 shows how the caller side will change due to this port.

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

Looks almost good to me. Just a few small tweaks and this should be good.

"once_cell",
"spin",
"untrusted",
"web-sys",
Copy link
Member

Choose a reason for hiding this comment

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

I don't get why they have web-sys here.

Copy link
Member Author

Choose a reason for hiding this comment

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

So it seems like deno_lockfile depends on ring, which in turn depends on web-sys (but this isn't included in the compilation, just included in the lock file). briansmith/ring#904 is talking about it

}

// clear the cache to reduce space for cache
api.clear_cache();
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove this from here and instead call it after running this function in the CLI

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, now clear_cache method is removed from the trait, and it's moved to the CLI code: https://github.com/denoland/deno/pull/20024/files#diff-747e5bf0658bef1ca03cfa6b83c21b7ffc899ce4324557b2cc94356681e76699R841

src/registry.rs Outdated
/// A trait for getting package information from the npm registry, with no
/// internal caching mechanism.
#[async_trait]
pub trait NpmRegistryApiWithoutCache: Sync + Send {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we could just have NpmRegistryApi to have default implementations that don't offer caching so we don't need an extra trait?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea!

/// The reason why this function is not combined with [`snapshot_from_lockfile`]
/// is because we want `lockfile` to not live across the `.await` points. This
/// becomes problematic if `lockfile` is wrapped in [`std::sync::Mutex`] or
/// similar, which doesn't implement [`Send`].
Copy link
Member

Choose a reason for hiding this comment

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

👍

@magurotuna magurotuna requested a review from dsherret August 3, 2023 10:24
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM!

@dsherret dsherret changed the title port snapshot_from_lockfile from denoland/deno feat: port snapshot_from_lockfile from denoland/deno Aug 3, 2023
@dsherret dsherret merged commit b2c06f3 into denoland:main Aug 3, 2023
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.

2 participants