feat: fix the chunk size warning when running vite build (resolves #32)#43
feat: fix the chunk size warning when running vite build (resolves #32)#43cindyli wants to merge 3 commits intoinclusive-design:mainfrom
vite build (resolves #32)#43Conversation
| */ | ||
| function initCellTypesSelect () { | ||
| const cellTypesSelect = document.getElementById("cellTypes"); | ||
| const cellTypesSelect = document.getElementById("cellTypes") as HTMLSelectElement; |
There was a problem hiding this comment.
Is the as HTMLSelectElement because VSCode's linter complains, where the npm run lint script does not? If so, that's interesting because I frequently find these type errors when writing and running tests. The test(s) fail with an error like cellTypesSelect has no add() method. After I add as HTMLSelectElement, the test runs without that error.
Note: because this is an app-demo script, we decided not the write tests for them, so this was not reported by the test framework either.
| labelledBy="slashExampleLabel" | ||
| /> | ||
| `, document.getElementById("slashExample")); | ||
| `, document.getElementById("slashExample") as HTMLElement); |
There was a problem hiding this comment.
The as HTMLElement is odd. The type of the second argument of render() is Element since the container can be an HTMLElement, an SVGSVGElement, a MathMLElement and other kinds of Elements. The return type for document.getElementById() is Element. Is this another place where VSCode's linter issued an error? In this case, I don't see why.
There was a problem hiding this comment.
Looking at the Type Assertions docs it would seem that HTMLElement would be inferred anyways. I think this information would be redundant unless there is a desire to be explicit here.
| * and create the PaletterStore and NavigationStack objects. | ||
| */ | ||
| export const adaptivePaletteGlobals = { | ||
| interface AdaptivePaletteGlobals { |
There was a problem hiding this comment.
A note to myself: this differs from the feat/integrate-ollama-api-with-palette in that the latter adds a systemPrompts dictionary member to the adaptivePaletteGlobals.
| * main paletted Defaults to the | ||
| * empty string which denotes | ||
| * the `<body>delement. | ||
| * the `<body>` element. |
There was a problem hiding this comment.
Thank you for catching my spelling errors and typos. I just noticed some more, a double "the the" above at line 94 and a "paletted" at 95.
|
|
||
| export async function loadBlissaryIdMap (): Promise<object> { | ||
| const response = await fetch(adaptivePaletteGlobals.blissaryIdMapUrl); | ||
| export async function loadDataFromUrl<T>(url: string): Promise<T> { |
There was a problem hiding this comment.
I can't remember why the original loadBlissaryIdMap() function was exported since the only place it is used is within GlobalData.ts. Similarly with its replacement loadDataFromUrl(). We should consider not exporting it. What do you think @cindyli ?
| paletteDisplay.innerText = "<p>Missing glosses ?</p>"; | ||
| } | ||
| const lookupResults = processPaletteLabels( | ||
| const lookupResults = await processPaletteLabels( |
There was a problem hiding this comment.
The await is not necessary. The following error is output from npm run serveAppsDemos:
✘ [ERROR] "await" can only be used inside an "async" function
apps/palette-generator/palette-generator.ts:126:24:
126 │ const lookupResults = await processPaletteLabels(
While processPaletteLabels() is actually declared as async, it does not appear to be asynchronous.
| * Helper function: Normalizes polymorphic return from decomposeBciAvId | ||
| * (value | array | undefined) into (array | undefined) | ||
| */ | ||
| const normalizeToOptionalArray = (val: unknown): (string | number)[] | undefined => { |
There was a problem hiding this comment.
This is cool. We might want to move this or some form of it to the GlobalUtils.ts. There are many places where I've had to add checks for a given BciAvIdType variable to see if it was the array or the number form and branch accordingly. I've often thought that there has to be a better way.
| }; | ||
|
|
||
| /** | ||
| * Helper function: Compares two arrays for equality safely |
There was a problem hiding this comment.
Maybe "Helper function: Compare two arrays for equality safely, where each item in the array is either a string or a number". What do you think?
| const wordMatch = new RegExp("\\b" + label + "\\b"); | ||
|
|
||
| if (label === gloss.description || wordMatch.test(gloss.description)) { | ||
| const glossId = typeof gloss.id === "number" ? gloss.id : parseInt(gloss.id); |
There was a problem hiding this comment.
I'm not sure that the check for a "number" type is needed. According to MDN, parseInt() first converts its argument to a string and then parses that. So passing an actual number is fine.
There was a problem hiding this comment.
You may also want to include the radix parameter to be explicit about what base the number has.
| }); | ||
| console.log(`\tFound match: ${gloss.description}, bci-av-id: ${gloss.id}`); | ||
|
|
||
| console.log(`\tFound match: ${gloss.description}, bci-av-id: ${glossId}`); |
There was a problem hiding this comment.
Regarding glossId in this log statement: what is the reason to use a number, where before it used the original string value gloss.id?
| let bciAvSymbolsDict: BciAvSymbolsDict = []; | ||
|
|
||
| // Load the BCI AV symbols dictionary from the URL | ||
| export async function loadBciAvSymbolsDict() { | ||
| bciAvSymbolsDict = await loadDataFromUrl<BciAvSymbolsDict>(bciAvSymbolsDictUrl); | ||
| } |
There was a problem hiding this comment.
I'm curious about this approach for a specific load function. It seem there are other functions in this file that also require the symbols.
I thought that import caches, so it only happens once across all the files. Does fetch also cache?
There was a problem hiding this comment.
I don't know about caching. I do know that a dynamic import using the import(...) function is implemented as fetch(), ultimately. Maybe static import is also implemented as fetch? In this case, the result of loadDataFromUrl() is stored in the variable bciAvSymbolsDict, which is how other functions within the file get access to the symbols. That variable is within the memory space of the web page -- is that memory cached anywhere?
The main reason for replacing static import with fetch is to avoid the build warning message that some chunks are too big. The build process puts data that is imported statically into the dist folder hierarchy. That does not happen for data that is accessed via fetch().
PS. In this case (line 27), loadDataFromUrl() is not replacing a static import. It's replacing a direct call to fetch(), if that matters.
3 participants
npm run testwithout errorsnpm run buildwithout errorsDescription
The chunk size warning message at running
vite buildwas caused by importing the largebliss_symbol_explanations.jsonfile. This pull request replace the import with a fetch from a raw github URL. With this fix, the json file is no longer required to be in the/publicdirectory.I use Chrome to test the load time before and after the fix. The load time is not affected much: 0.32s for before, 0.34s for after.
While working on this issue, I noticed my VS Code editor reports many typescript error complaining the undefined types on many variables. I fixed those in scripts I modified. However, there are more in other scripts.
Steps to test
npm startin a terminalExpected behavior: Make sure all functionality works as is.