sewing-kit-koa now checks for matching locales #1199
Conversation
GoodForOneFare
changed the title
GoodForOneFare
force-pushed
the
multi-locale-koa
branch
from
853f7ef
to
2876a5b
Compare
There was a problem hiding this comment.
Code looks good to me, I had a few small nits and questions.
I'm also curious of the performance impact on the middleware itself (ever since that one time we accidentally made the sewing_kit equivalent way slower I've been a bit more paranoid about making these thing slow). I'd expect it would only add a few ms, but on an app like web with many languages we might want to be aware of how it changes, especially once there are 100+. Do we have numbers or anything for that?
| acc: Map<string, ConsolidatedManifest>, | ||
| manifest: Manifest, | ||
| ) { | ||
| const locales = manifest.identifier!.locales; |
There was a problem hiding this comment.
is identifier really optional? If not why don't we change the type (it's from this library isn't it?) if so, we should maybe do an assertion above with a nice error message if it's not there?
There was a problem hiding this comment.
It's optional for now because identifier's presence depends on an unreleased version of sewing-kit 😅
The mandatory part can come 🔜 🤞
| export default class Assets { | ||
| assetPrefix: string; | ||
| userAgent?: string; | ||
| manifestPath: string; | ||
| private manifests: Manifests; | ||
| private resolvedManifestEntry?: Manifest; | ||
|
|
||
| constructor({assetPrefix, userAgent, manifestPath}: Options) { |
There was a problem hiding this comment.
Given that we create one instance of this per request is there a reason we don't add locale to the class itself? Then we could remove the params all over the place for locale and just use the class attribute. Feels weird that we have a mix of params and class attributes for things that are per request.
There was a problem hiding this comment.
Yah, this is nasty
Locales are a niche feature right now, and I figure it's better to just add this verbose version of things before we put any more work into the API.
The cost is all up front: read a larger manifest on first request, and sort it into buckets. For every other request:
So it feels to me like the runtime cost is one extra map access and maybe an extra function call ‾\(ツ)/‾ |
GoodForOneFare
force-pushed
the
multi-locale-koa
branch
from
2ba0591
to
18f4f04
Compare
GoodForOneFare
force-pushed
the
multi-locale-koa
branch
from
18f4f04
to
27d1c18
Compare
Description
When provided with manifests containing a
localeattribute, sewing-kit-koa will now attempt to match a user request to a set of assets with that locale's translations embedded into its JavaScript.This is a companion to the react-i18n PR that supports per-locale builds: #1197. The advantages of this are:
Type of change
@shopify/sewing-kit-koaMinor: New feature (non-breaking change which adds functionality)Checklist