feat: add detection + nice error message for npm import errors #195
Conversation
Skn0tt
added
the
type: feature
node/bundler.test.ts
Outdated
| expect(error).toBeInstanceOf(NPMImportError) | ||
| expect((error as NPMImportError).moduleName).toEqual('p-retry') | ||
| expect((error as NPMImportError).message).toEqual( | ||
| `It seems like you're trying to import an NPM module, did you mean to 'import ... from "npm:p-retry"'?`, |
There was a problem hiding this comment.
We shouldn't offer this suggestion because don't support it.
|
|
||
| const checkNpmImportError = (input: unknown): NPMImportError | undefined => { | ||
| if (input instanceof Error) { | ||
| const match = input.message.match(/Relative import path "(.*)" not prefixed with/) |
There was a problem hiding this comment.
Matching the error message feels a bit brittle. As I suggested here, we could leverage the fact that we're in control of the module resolving (in production anyway, where we use ESZIP).
We should be able to look at a specifier and see whether it's a full URL, a relative path, or anything else. If it's anything else and the loading fails, I think we can safely infer that people are either trying to load an npm module or a specifier that should be in the import map but isn't.
I think doing that check in the loader would be more robust.
There was a problem hiding this comment.
I tried doing that check in the loader, but it doesn't seem like the loader is ever called with p-retry. It's called once with netlify:bootstrap-stage2, and once with file:///root/func1.ts, and then the resolution mechanism throw the error Relative import path ... not prefixed with .... So I don't see a way of detecting this within the loader. Maybe i'm missing something? Would be happy for your help on this.
There was a problem hiding this comment.
Ah, interesting! I thought the ESZIP loader would be called even for paths that don't match a URL format or a path. In that case, ignore me!
node/formats/eszip.ts
Outdated
| @@ -52,6 +53,11 @@ const bundleESZIP = async ({ | |||
| try { | |||
| await deno.run(['run', ...flags, bundler, JSON.stringify(payload)], { pipeOutput: true }) | |||
| } catch (error: unknown) { | |||
| const npmImportError = checkNpmImportError(error) | |||
| if (npmImportError) { | |||
| throw npmImportError | |||
There was a problem hiding this comment.
wrapBundleError is responsible for setting the customErrorInfo property, which is then used to distinguish user errors from system errors. By not wrapping npmImportError, we'll start incorrectly treating these as system errors.
Co-authored-by: Eduardo Bouças <[email protected]>
node/formats/eszip.ts
Outdated
| } catch (_error: unknown) { | ||
| let error = _error | ||
| error = checkNpmImportError(error) ?? error |
There was a problem hiding this comment.
I find this sequence confusing, to be honest.
I think a method called checkNpmImportError that returns either an error or undefined isn't the most natural?
I would either:
- Call it
isNpmImportErrorand make it return a boolean — you'd then need to call theNPMImportErrorseparately if that condition is true, or - Call it something like
wrapNpmImportErrorwhich always returns an error — either the original error if it's not an npm import error, or an instance ofNPMImportErrorif it is
What do you think?
There was a problem hiding this comment.
Agreed! looks like I write too much go :D changed in 67475dd
| class NPMImportError extends Error { | ||
| constructor(originalError: Error, moduleName: string) { | ||
| super( | ||
| `It seems like you're trying to import an npm module. This is only supported in Deno via CDNs like esm.sh. Have you tried 'import mod from "https://esm.sh/${moduleName}"'?`, |
There was a problem hiding this comment.
Btw, extracting the module name with the regular expression and showing it here is a really nice touch! ✨
|
we should really install a pre-commit hook with prettier ... |
…fy/edge-bundler#195) * feat: add detection + nice error message for npm import errors * fix: update recommendation * fix: fmt * fix: remove .only * feat: also detect npm imports for eszip builds * Update node/npm_import_error.ts Co-authored-by: Eduardo Bouças <[email protected]> * fix: test * fix: wrap in BundleError * chore: add comment about user-errors * refactor: wrapImportError * fix: prettier Co-authored-by: Eduardo Bouças <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Closes https://github.com/netlify/pod-compute/issues/287