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

SDK - Module Caching #2940

Merged

Conversation

LorisSigrist
Copy link
Member

@LorisSigrist LorisSigrist commented Jun 17, 2024

This PR adds a network-first module cache to the SDK. This enables devs to keep working offline & in case JSDelivr goes down.

In case the network isn't available, or incredibly slow, it can be annoying to have to wait for the request timeout. To speed this up the SDK now fetches modules in parallel instead of sequentially.

Copy link

changeset-bot bot commented Jun 17, 2024

🦋 Changeset detected

Latest commit: 9d7e7fe

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 25 packages
Name Type
@inlang/sdk Minor
@inlang/badge Patch
@inlang/doc-layout-component Patch
@inlang/editor Patch
@inlang/github-lint-action Patch
vs-code-extension Patch
@inlang/message-bundle-component Patch
@inlang/rpc Patch
@inlang/settings-component Patch
@inlang/telemetry Patch
@inlang/cross-sell-ninja Patch
@inlang/plugin-i18next Patch
@inlang/plugin-json Patch
@inlang/plugin-m-function-matcher Patch
@inlang/plugin-next-intl Patch
@inlang/plugin-t-function-matcher Patch
@inlang/paraglide-unplugin Patch
@inlang/paraglide-js-e2e Patch
@inlang/paraglide-next-e2e Patch
@inlang/paraglide-rollup Patch
@inlang/paraglide-vite Patch
@inlang/paraglide-webpack Patch
@inlang/paraglide-astro Patch
@inlang/paraglide-sveltekit Patch
@inlang/paraglide-sveltekit-example Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@LorisSigrist
Copy link
Member Author

Questions for Reviewers:

  • Is "Network first" the right strategy?
  • How should we purge the cache of removed modules (it currently doesn't). IMO we can do this later since modules are rarely removed & barely use any disk space
  • How can we test offline usage?

@jldec
Copy link
Contributor

jldec commented Jun 17, 2024

Thanks for working on this @LorisSigrist! The implementation appears to work as intended. 👍

I have an offline compile working in a test paraglide-sveltekit project, by running the build once, and then disconnecting 🎉

I haven't looked at the code in detail yet, but here are some initial comments after testing:

  1. how about generating the .gitignore also during paraglide-js init and in createNewProject to avoid the subsequent additional commit.
  2. the cache filenames are rather long (180+ characters) - any chance of reducing that.
  3. I see typescript errors in the sdk/load-test project after the first run because it doesn't exclude project.inlang - any ideas for how to avoid this in other projects.
    (you can repro by running the following command twice: pnpm run --filter @inlang/sdk-load-test test)
  4. I agree that we can defer the cache removal.
  5. I didn't observe much of delay from the "network first" approech when running offline, so I think that's ok, to keep the modules fresh if users are using @latest. I do see a long pause at the end, after compiling while offline, but I think that may be coming from our telemetry or something else.

@jldec
Copy link
Contributor

jldec commented Jun 17, 2024

I pushed my test repo to https://github.com/jldec/svelte-latest
For testing with this PR branch, it needs to be cloned in a peer directory, alongside our monorepo. E.g

/opral/monorepo
/opral/svelte-latest

Copy link
Member

@samuelstroschein samuelstroschein left a comment

Choose a reason for hiding this comment

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

Network-first avoids cache invalidation problems. Hence, good choice to ship this fast. Can be improved later.

Minor comments. Implement them or not. Your choice.

Copy link
Contributor

@jldec jldec left a comment

Choose a reason for hiding this comment

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

LGTM - see comment regarding checking the FNV implementation

fyi - i'd be fine with slightly longer file names (10-40 chars) if that gave us better collision avoidance or makes the filenames nicer e.g. if you want to use 52 bit from https://github.com/tjwebb/fnv-plus

@LorisSigrist LorisSigrist merged commit 2fa96b3 into main Jun 18, 2024
3 checks passed
@LorisSigrist LorisSigrist deleted the lorissigrist/mesdk-81-requirements-for-module-caching branch June 18, 2024 09:28
@github-actions github-actions bot locked and limited conversation to collaborators Jun 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants