feat: dynamic import retry plugin [KM-865] #2

2eha0 · 2025-01-07T07:36:00Z

Summary

add dynamic import retry plugin
add e2e tests and vitest settings
modified CI to support playwright caches

adamdehaven

I left an initial review; the biggest issue is please back out the changes turning this into a mono repo.

Separately, it would be great if you could please extensively comment the code so that maintenance and future updates are easier.

package.json

playground/vitestGlobalSetup.ts

adamdehaven · 2025-01-07T14:37:53Z

playground/vitestSetup.ts

@@ -0,0 +1,350 @@
+import type * as http from 'node:http'


question: I believe most of this was taken from one of the existing vite repos. Are we actually using the entirety of the config or is there any unneeded code here for our purpose?

I removed most of code that I thought they were not necessary for us.

playground/vue/.gitignore

playground/vitestSetup.ts

adamdehaven · 2025-01-07T14:40:49Z

playground/vue/package.json

+{
+  "name": "@kong-vite-plugin/vue",
+  "private": true,
+  "type": "module",
+  "scripts": {
+    "dev": "vite",
+    "build": "vue-tsc -b && vite build",
+    "preview": "vite preview"
+  },
+  "dependencies": {
+    "vue": "^3.5.13",
+    "vue-router": "^4.5.0"
+  },
+  "devDependencies": {
+    "@vitejs/plugin-vue": "^5.2.1",
+    "@vue/tsconfig": "^0.7.0",
+    "vue-tsc": "^2.2.0"
+  }
+}


issue: why does the playground need its own package.json file? This isn't a monorepo. Please move dependencies to the root

We can create multiple projects under the playground folder, package.json for each one is necessary, otherwise, how can we manually run those demo projects from the root level?

Can you please write up how you envision the testing working?

I’m not sure it makes sense to have fully-standalone applications for testing purposes.

If we need to change to a mono repository then I’d first want to restructure things before merging this PR

If each package is independent and needs an entire app sandbox I’d almost rather delete this repository and just utilize public-ui-components instead; it’s already set up and would essentially just need a different build command.

For dependencies in the current structure, playground dependencies would be devDependencies in the root package.json

My initial thought was that we might need multiple applications to test different cases, similar to https://github.com/vitejs/vite-plugin-vue/tree/main/playground.

However, we can start simple by writing all use cases in a single project. If more complex scenarios arise, we can refactor it into a monorepo as needed.

playground/vue/tsconfig.json

pnpm-workspace.yaml

adamdehaven · 2025-01-07T14:48:43Z

src/plugin-dynamic-import-retry/import-wrapper.ts

+  /*
+  Copyright 2024 Carl-Erik Kopseng
+
+  Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the “Software”), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:
+
+  The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.
+  */


is this a requirement of an external lib? If yes, can you please add a link to the source?

Also, could this be extracted into its own file and imported where needed rather than inlined into our code?

link added.

No, the code in this function shouldn't have external references, I put detailed reason in comments.

.github/actions/setup-pnpm-with-dependencies/action.yaml

CLAassistant · 2025-01-08T03:10:15Z

All committers have signed the CLA.

adamdehaven

Left some initial feedback.

There is a large lack of comments in the code that would be helpful for review. Please thoroughly comment the code.

adamdehaven · 2025-01-10T02:58:45Z

.github/actions/setup-pnpm-with-dependencies/action.yaml

+      id: playwright-cache
+      uses: actions/cache@v4
+      with:
+        key: playwright-bin-v1


Should we tie the cache key here to a dynamic string that includes the playwright dependency version from package.json?

Good point! Updated.

.gitignore

playground/dynamic-import-retry/vue-router/src/vite-env.d.ts

adamdehaven · 2025-01-10T03:03:26Z

playground/dynamic-import-retry/vue-router/tsconfig.json

+    "baseUrl": ".",
+    "outDir": "dist",
+    "declaration": false,
+    "declarationDir":null


Suggested change

"declarationDir":null

"declarationDir": null

@adamdehaven Maybe we should enforce this with our shared ESLint config?

src/plugin-dynamic-import-retry/import-wrapper.ts

Justineo · 2025-01-07T09:03:39Z

src/plugin-dynamic-import-retry/css-preload-retry.ts

+    const newError = new Error(`[preload-css-retried] ${e.payload.message}`)
+    throw newError


Suggested change

const newError = new Error(`[preload-css-retried] ${e.payload.message}`)

throw newError

throw new Error(`[preload-css-retried] ${e.payload.message}`)

Justineo · 2025-01-10T04:56:42Z

playground/dynamic-import-retry/vue-router/tsconfig.json

+    "baseUrl": ".",
+    "outDir": "dist",
+    "declaration": false,
+    "declarationDir":null


@adamdehaven Maybe we should enforce this with our shared ESLint config?

src/plugin-dynamic-import-retry/README.md

adamdehaven · 2025-01-13T12:13:51Z

@2eha0 why are you force pushing the branch? You’re obliterating the commit history and it’s impossible to see the changes between commits

adamdehaven · 2025-01-13T12:48:57Z

Separate from the changes in the PR, have we done any benchmarking to determine if implementing this plugin in a host application has any performance / timing impacts in the build, or the actual imports themselves?

2eha0 · 2025-01-14T03:54:01Z

Runtime cost

This plugin will only work if the module fails to load, which is just one more js function call for the normal case and has no impact on performance.

the pseudocode:

function importWraper(importFn) {
  try {
    await importFn() // <- the normal case
  } catch () {
   // do retry... // <- only run here on error
  }
}

importWraper(() => import('xxxx')) // original: () => import('xxx')

Comparison of time spent during build

There's no discernible difference.

With out the plugin	With the plugin
1m 18s	1m 22s

https://github.com/Kong/konnect-ui-apps/actions/runs/12758610182/job/35561049472

https://github.com/Kong/konnect-ui-apps/actions/runs/12760560701/job/35566279787?pr=5332

Justineo · 2025-01-14T06:16:16Z

I think the performance impact should be rather limited and acceptable.

adamdehaven · 2025-01-15T14:42:56Z

We should also investigate how this plugin could impact usage of something like defineAsyncComponent where the import, loading, and error state is handled as part of the implementation: https://vuejs.org/guide/components/async.html#loading-and-error-states

adamdehaven · 2025-01-15T14:47:48Z

Does this plugin, by default, run only against the source code in the repository, or does it also run against all node_modules, etc.?

My gut is telling me we would want to exclude node_modules by default since other packages may have their own internal optimizations, desired behavior, etc.

We could then potentially allow opting in based on a package name, similar to how rollup marks externals or Vite does optimizeDeps

adamdehaven · 2025-01-15T14:49:00Z

src/plugin-dynamic-import-retry/README.md

+| include  | string \| RegExp \| (string \| RegExp)[]  | Files to include, default is `\.(js\|ts\|vue\|tsx)$`.          |
+| exclude  | string \| RegExp \| (string \| RegExp)[]  | Files to exclude.                                              |


We should likely exclude node_modules by default, and then allow opting-in where needed.

We should discuss any other desired default configurations

I thought about it again today, and I still can’t figure out in what scenarios we would prefer to display an error instead of retrying and then falling back to an exception state when a resource chunk fails to load. In cases like defineAsyncComponent with a timeout set, as I understand it, retrying wouldn’t extend the wait time—the timeout would still display an error as expected. If we were to use a dynamic import for something like “feature detection”, it doesn’t seem likely that the distinction would be based on whether the imported module resides in node_modules. Can you think of any practical examples where retrying would negatively impact the original logic?

I'm more suggesting it may be worth testing so we know how it will behave rather than assuming

Sure. Maybe we can add a test case for defineAsyncComponent? @2eha0

test cases for defineAsyncComponent have been added.

package.json

adamdehaven · 2025-01-21T14:11:48Z

When the plugin retries the import, does it typically always succeed?

I started wondering if the issue is actually rooted in the way we invalidate the cache on AWS. The first request after the cache is invalidated may fail, but would trigger AWS to put the requested file back in the cache. A subsequent request would then have a cache hit, and would successfully download the chunk (similar to SWR behavior).

Could we validate this isn't the case in the deployed MFE?

2eha0 · 2025-01-22T03:35:14Z

I don’t think this is how the CDN is supposed to work; resources should always be served, even without caches. If it does, our Prod regression tests should always fail.

Something I feel strange about AWS is sometimes I get 403 when I download JS files, I think it might be related to our settings in AWS?

Justineo · 2025-01-22T03:52:04Z

I started wondering if the issue is actually rooted in the way we invalidate the cache on AWS. The first request after the cache is invalidated may fail, but would trigger AWS to put the requested file back in the cache. A subsequent request would then have a cache hit, and would successfully download the chunk (similar to SWR behavior).

I believe AWS would not reject requests during revalidation unless the origin is unavailable itself. Additionally, I believe that our JavaScript files shouldn’t invalidate at all, as they all have content hashes in the filenames.

adamdehaven · 2025-01-22T04:07:13Z

I believe that our JavaScript files shouldn’t invalidate at all, as they all have content hashes in the filenames.

We manually invalidate the cache for some paths, although not JS that I recall (example)

…retry

adamdehaven · 2025-02-05T03:16:38Z

.github/actions/setup-pnpm-with-dependencies/action.yaml

+    - name: Set Playwright path
+      id: playwright-path
+      shell: bash
+      run: echo "PLAYWRIGHT_BROWSERS_PATH=/home/runner/.cache/ms-playwright" >> $GITHUB_OUTPUT


Two issues:

I believe you actually need to expose this variable to the $GITHUB_ENV and then pass this env variable to the step that runs the install command so that Playwright will utilize it during the install

I believe the path should be this:

Suggested change

run: echo "PLAYWRIGHT_BROWSERS_PATH=/home/runner/.cache/ms-playwright" >> $GITHUB_OUTPUT

run: echo "PLAYWRIGHT_BROWSERS_PATH=/home/ubuntu/.cache/ms-playwright" >> $GITHUB_ENV

Done, and I also replaced /home to $HOME

adamdehaven · 2025-02-05T03:17:34Z

.github/actions/setup-pnpm-with-dependencies/action.yaml

+      id: playwright-install
+      shell: bash
+      # does not need to explicitly set chromium after https://github.com/microsoft/playwright/issues/14862 is solved
+      run: pnpm playwright install chromium


You need to pass the PLAYWRIGHT_BROWSERS_PATH here as an env variable

ValeryG · 2025-02-05T14:18:08Z

I don’t think this is how the CDN is supposed to work; resources should always be served, even without caches. If it does, our Prod regression tests should always fail.

Something I feel strange about AWS is sometimes I get 403 when I download JS files, I think it might be related to our settings in AWS?

@2eha0 , 403 is getting returned when non-existing asset is requested: eg: https://cloud.konghq.tech/signin/assets/bad-asset-name.js

if you see this happening for assets that are valid and should be returned with 200, please give me an example of those requests.

ValeryG · 2025-02-05T14:20:42Z

I started wondering if the issue is actually rooted in the way we invalidate the cache on AWS. The first request after the cache is invalidated may fail, but would trigger AWS to put the requested file back in the cache. A subsequent request would then have a cache hit, and would successfully download the chunk (similar to SWR behavior).

I believe AWS would not reject requests during revalidation unless the origin is unavailable itself. Additionally, I believe that our JavaScript files shouldn’t invalidate at all, as they all have content hashes in the filenames.

That is correct, if asset is NOT on the edge, request is going to origin , grabs asset from there , spits it back to the requestor and sticks it into CDN. There is never the case when error is returned when asset is on the origin, but not on the edge.

ValeryG · 2025-02-05T14:23:30Z

question: let's assume this is enabled for MFE. Is there any way to see in the datadog:

how many times retry needed to be invoked
how many times the logic of retry safes the world and asset request actual recovers

This allows us to evaluate the usefulness of this plugin :)

src/plugin-dynamic-import-retry/index.ts

Justineo · 2025-02-05T15:15:11Z

@2eha0 had added two widgets to our DD dashboard (one for JS errors and the other for CSS errors) for current experiments in gateway-manager MFE. @ValeryG

Once the plugin is published, we’ll install it in gateway-manager first and monitor the data for a while before enabling it for the other MFEs.

adamdehaven · 2025-02-05T15:29:54Z

@2eha0 had added two widgets to our DD dashboard (one for JS errors and the other for CSS errors) for current experiments in gateway-manager MFE. @ValeryG

Once the plugin is published, we’ll install it in gateway-manager first and monitor the data for a while before enabling it for the other MFEs.

I still feel like we may be over-optimizing here TBH.

Justineo · 2025-02-05T15:42:15Z

This isn’t exactly a major pain point, but it’s at least a nice-to-have. The experiment has shown that it is indeed effective, and the community does have similar needs: whatwg/html#6768.

2eha0 force-pushed the feat/dynmamic-import-retry branch 8 times, most recently from 77c17e6 to 9ddda00 Compare January 7, 2025 08:37

adamdehaven reviewed Jan 7, 2025

View reviewed changes

.github/actions/setup-pnpm-with-dependencies/action.yaml Outdated Show resolved Hide resolved

2eha0 force-pushed the feat/dynmamic-import-retry branch from c0a326e to 73dc74e Compare January 9, 2025 09:41

adamdehaven requested changes Jan 10, 2025

View reviewed changes

Justineo reviewed Jan 13, 2025

View reviewed changes

feat: dynamic import retry plugin [KM-865]

7cc82d4

2eha0 force-pushed the feat/dynmamic-import-retry branch from 1f0c3c4 to 7cc82d4 Compare January 13, 2025 03:48

2eha0 marked this pull request as ready for review January 15, 2025 02:24

2eha0 requested review from kongponents-bot, jillztom, ValeryG and a team as code owners January 15, 2025 02:24

adamdehaven reviewed Jan 15, 2025

View reviewed changes

feat: default exclude node_modules

b693bae

Justineo reviewed Jan 17, 2025

View reviewed changes

package.json Show resolved Hide resolved

2eha0 added 4 commits January 17, 2025 13:29

test: vue async component

4f4a0e2

fix: lint warn

d9d2e53

fix: remove unused file

4142f22

test: transitive and dynamic-import-vars

2cfa2ad

Merge remote-tracking branch 'origin/main' into feat/dynmamic-import-…

ef8837b

…retry

adamdehaven reviewed Feb 5, 2025

View reviewed changes

2eha0 added 5 commits February 5, 2025 11:31

fix: playwright cache

00a7462

fix: playwright cache

55c1300

fix: playwright cache

385164f

ci: test cache

74a06e6

fix: cache key

7b3806e

adamdehaven reviewed Feb 5, 2025

View reviewed changes

src/plugin-dynamic-import-retry/index.ts Show resolved Hide resolved

		const newError = new Error(`[preload-css-retried] ${e.payload.message}`)
		throw newError

	const newError = new Error(`[preload-css-retried] ${e.payload.message}`)
	throw newError
	throw new Error(`[preload-css-retried] ${e.payload.message}`)

		\| include \| string \\| RegExp \\| (string \\| RegExp)[] \| Files to include, default is `\.(js\\|ts\\|vue\\|tsx)$`. \|
		\| exclude \| string \\| RegExp \\| (string \\| RegExp)[] \| Files to exclude. \|

	run: echo "PLAYWRIGHT_BROWSERS_PATH=/home/runner/.cache/ms-playwright" >> $GITHUB_OUTPUT
	run: echo "PLAYWRIGHT_BROWSERS_PATH=/home/ubuntu/.cache/ms-playwright" >> $GITHUB_ENV

feat: dynamic import retry plugin [KM-865] #2

Are you sure you want to change the base?

feat: dynamic import retry plugin [KM-865] #2

Conversation

2eha0 commented Jan 7, 2025 • edited by jira bot Loading

Summary

adamdehaven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamdehaven Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Jan 8, 2025 • edited Loading

adamdehaven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamdehaven commented Jan 13, 2025

adamdehaven commented Jan 13, 2025

2eha0 commented Jan 14, 2025

Runtime cost

Comparison of time spent during build

Justineo commented Jan 14, 2025

adamdehaven commented Jan 15, 2025

adamdehaven commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamdehaven commented Jan 21, 2025

2eha0 commented Jan 22, 2025

Justineo commented Jan 22, 2025

adamdehaven commented Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValeryG commented Feb 5, 2025

ValeryG commented Feb 5, 2025 • edited Loading

ValeryG commented Feb 5, 2025

Justineo commented Feb 5, 2025

adamdehaven commented Feb 5, 2025

Justineo commented Feb 5, 2025

2eha0 commented Jan 7, 2025 •

edited by jira bot

Loading

adamdehaven Jan 7, 2025 •

edited

Loading

CLAassistant commented Jan 8, 2025 •

edited

Loading

adamdehaven commented Jan 22, 2025 •

edited

Loading

ValeryG commented Feb 5, 2025 •

edited

Loading