Fix invalid JavaScript in readme example #102
Conversation
This switch/typeof example isn't valid JavaScript AFAICT -- JS doesn't have "type switches" like Go does. NOTE: Unfortunately this example still won't work, because NotFoundError and MethodNotAllowedError are not imported. It doesn't look like kv-asset-handler exports these types currently, so I guess a code change is needed to export them? See cloudflare#93 (not fully fixed by this PR)
|
I tested importing in the sites-template and it did work for me. (For reference https://gist.github.com/victoriabernard92/c55bb17e7e15dfa6612e7675286c9422) Is it not working for you? Either way, I think we should fix it to use import instead. |
|
I didn't try it, but I looked at the code, and it doesn't appear to export the error types, so I assumed that wouldn't work... https://github.com/cloudflare/kv-asset-handler/blob/master/src/index.ts#L193 I'm not sure what happens if you import a symbol that wasn't exported. Maybe it produces Have you tested that the |
|
FWIW it looks like Another problem I just realized is that the switch is on |
|
Wow, actually, |
|
... I wonder if importing |
Yes, it does. |
|
@SukkaW Sorry, I think you misunderstood what I meant. I was wondering if it was possible that the original code -- if only the missing import was fixed -- would accidentally always execute the However, I tested this and it does not appear to be the case, probably because |
|
Is there anything wrong with just adding the export for the types? |
|
@victoriabernard92 We need to do all of:
|
|
would probably be good to just move the example out of the readme and put it in |
| default: | ||
| return new Response("An unexpected error occurred", { status: 500 }) | ||
| } | ||
| if (e instanceof NotFoundError) { |
There was a problem hiding this comment.
The correct case should be:
import { getAssetFromKV } from '@cloudflare/kv-asset-handler'
import { NotFoundError, MethodNotAllowedError } from '@cloudflare/kv-asset-handler/dist/error'And error.js should export as well. Otherwise the instanceof won't work.
There was a problem hiding this comment.
I'm not sure if /dist/error is intended to be imported from outside the package. I think we need to re-export the error types from index.js.
In order to publish types, the minimal change required is: * Update `index.ts` to also export `Options, CacheControl, MethodNotAllowedError, NotFoundError, InternalError` `npm run build` (which runs `tsc -d`, where the `-d` flag indicates to generate type definition files) I observed that this generates a `dist/index.d.ts` type definition file that contains all types except those defined in `src/mock.ts`, which are only required for package tests and thus don’t need to be published. `package.json` already specifies these as the package’s types: ```json "main": "dist/index.js", "types": "dist/index.d.ts", ``` As a side note, I’m not sure why `index.d.ts` (even with a subset of types) isn’t included in the `0.0.10` release. If I check out the `0.0.10` tag and run `npm publish —dry-run` I observe that it includes `dist/index.d.ts` in the package. Finally, I tested that this works using `npm link`. I created a new typescript project containing the updated sample code from @kentonv’s PR against `README.md` in [Fix invalid JavaScript in readme example by kentonv · Pull Request #102 · cloudflare/kv-asset-handler · GitHub](https://github.com/cloudflare/kv-asset-handler/pull/102/files#diff-04c6e90faac2675aa89e2176d2eec7d8), then ran: ```bash npm i @cloudflare/kv-asset-handler --save npm link @cloudflare/kv-asset-handler ``` This symlinks to my local copy of `kv-asset-handler` instead of the 0.0.10 release on npm. After doing so, I was able to do the following in said new project: ```typescript import { getAssetFromKV, NotFoundError, MethodNotAllowedError } from '@cloudflare/kv-asset-handler' ``` I validated that the new test code transpiles and runs when using these typings.
|
@kentonv 's new snippet should work now that I merged #106 . I think we should just merge this to make it work, and include tested examples as a follow-up improvement (good idea @EverlastingBugstopper ). |
|
@ispivey Done |
5 participants
This switch/typeof example isn't valid JavaScript AFAICT -- JS doesn't have "type switches" like Go does.
NOTE: Unfortunately this example still won't work, because NotFoundError and MethodNotAllowedError are not imported. It doesn't look like kv-asset-handler exports these types currently, so I guess a code change is needed to export them?
See #93 (not fully fixed by this PR)