Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle error in numberToPos and formatError #4782

Merged
merged 2 commits into from Sep 1, 2021

Conversation

milahu
Copy link
Contributor

@milahu milahu commented Aug 29, 2021

old behavior

  1. formatError is trying to handle error1
  2. formatError calls numberToPos
  3. numberToPos throws error2
  4. only error2 is printed
  5. error1 is not printed

for example

Error: offset is longer than source length!
    at numberToPos (node_modules/vite/dist/node/chunks/dep-972722fa.js:4247:15)
    at formatError (node_modules/vite/dist/node/chunks/dep-972722fa.js:51169:24)
    at TransformContext.error (node_modules/vite/dist/node/chunks/dep-972722fa.js:51149:19)
    at Object.transform (node_modules/vite/dist/node/chunks/dep-972722fa.js:51357:25)
    at async transformRequest (node_modules/vite/dist/node/chunks/dep-972722fa.js:67098:29)
    at async instantiateModule (node_modules/vite/dist/node/chunks/dep-972722fa.js:73732:10)

new behavior

formatError: error in numberToPos (Error: offset is longer than source length! offset 126 > length 48) while handling the error:
Expected valid tag name
4: </script>
5: 
6: <p>Lorem ipsum < dolor sit</p>
                   ^
ParseError: Expected valid tag name
    at error (node_modules/svelte/compiler.mjs:17698:19)
    at Parser$1.error (node_modules/svelte/compiler.mjs:17774:9)
    [...]

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@milahu milahu changed the title handle errors in numberToPos and formatError: offset is longer than source length handle error in numberToPos and formatError: offset is longer than source length Aug 29, 2021
milahu added a commit to milahu/vite that referenced this pull request Aug 30, 2021
milahu added a commit to milahu/vite that referenced this pull request Aug 30, 2021
deprecated acorn-numeric-separator@0.3.6: acorn>=7.4 supports numeric separators
milahu added a commit to milahu/vite that referenced this pull request Aug 30, 2021
package.json Outdated Show resolved Hide resolved
packages/vite/package.json Show resolved Hide resolved
milahu added a commit to milahu/vite that referenced this pull request Aug 30, 2021
milahu added a commit to milahu/vite that referenced this pull request Aug 30, 2021
deprecated acorn-numeric-separator@0.3.6: acorn>=7.4 supports numeric separators
milahu added a commit to milahu/vite that referenced this pull request Aug 30, 2021
milahu added a commit to milahu/vite that referenced this pull request Aug 30, 2021
@milahu milahu changed the title handle error in numberToPos and formatError: offset is longer than source length fix: handle error in numberToPos and formatError: offset is longer than source length Aug 30, 2021
Comment on lines 286 to 297
let errLocation;
try {
errLocation = numberToPos(ctx._activeCode, pos);
}
catch (err2) {
logger.error(
chalk.red(`Error in error handler:\n${err2.stack || err2.message}`),
{ error: err2 }
)
// TODO verify that err is visible
throw err;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO verify that err is visible

Copy link
Contributor Author

@milahu milahu Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how to add a unit test for this?
vite.createServer -> createPluginContainer -> formatError

broken code is passed to the plugin -> plugin throws error1
plugin returns a broken sourcemap -> numberToPos throws error2 "offset is longer than source length"

scripts/jestPerTestSetup.ts calls server = await (await createServer(options)).listen()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patak-js Do we even test logged errors?
I'm not sure if we need to test this 🤔

yarn.lock Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 added the p1-chore Doesn't change code behavior (priority) label Aug 30, 2021
@Shinigami92 Shinigami92 self-requested a review August 30, 2021 15:14
@Shinigami92 Shinigami92 marked this pull request as draft August 30, 2021 15:14
@Shinigami92
Copy link
Member

Converted to draft until TODO verify that err is visible is resolved

@milahu
Copy link
Contributor Author

milahu commented Aug 30, 2021

TODO verify that err is visible

done manually, this is non-trivial to test

note: this is NOT visible in npm run build

$ npm run dev 

> @ dev svelte/mdsvex/sveltekit-bugs
> svelte-kit dev

Pre-bundling dependencies:
  svelte/store
  svelte
  svelte/animate
  svelte/easing
  svelte/internal
  (...and 2 more)
(this will be run only when your dependencies or config have changed)

  SvelteKit v1.0.0-next.159

  local:   http://localhost:3000
  network: not exposed

  Use --host to expose server to other devices on this network


Error in error handler:
Error: offset is longer than source length! offset 126 > length 48
    at numberToPos (vite/dist/node/chunks/dep-7017d082.js:4256:15)
    at formatError (vite/dist/node/chunks/dep-7017d082.js:50004:35)
    at TransformContext.error (vite/dist/node/chunks/dep-7017d082.js:49984:19)
    at Object.transform (vite/dist/node/chunks/dep-7017d082.js:50200:25)
    at async transformRequest (vite/dist/node/chunks/dep-7017d082.js:64964:29)
    at async instantiateModule (vite/dist/node/chunks/dep-7017d082.js:74748:10)

Expected valid tag name
4: </script>
5: 
6: <p>Lorem ipsum < dolor sit</p>
                   ^
ParseError: Expected valid tag name
    at error (svelte/compiler.mjs:17697:19)
    at Parser$1.error (svelte/compiler.mjs:17773:9)
    at read_tag_name (svelte/compiler.mjs:16918:16)
    at tag (svelte/compiler.mjs:16768:18)
    at new Parser$1 (svelte/compiler.mjs:17732:21)
    at parse$3 (svelte/compiler.mjs:17864:20)
    at compile (svelte/compiler.mjs:31303:17)
    at compileSvelte (vite-plugin-svelte/dist/index.js:301:20)
    at async TransformContext.transform (vite-plugin-svelte/dist/index.js:1325:27)
    at async Object.transform (vite/dist/node/chunks/dep-7017d082.js:50197:30)

@Shinigami92 Shinigami92 marked this pull request as ready for review August 30, 2021 17:04
@milahu
Copy link
Contributor Author

milahu commented Aug 30, 2021

note: currently, the browser runtime shows only the second error

500

Expected valid tag name 4: </script> 5: 6: <p>Lorem ipsum < dolor sit</p> ^ ParseError: Expected valid tag name at error (svelte/compiler.mjs:17697:19) at Parser$1.error (svelte/compiler.mjs:17773:9) [...]

@patak-dev patak-dev changed the title fix: handle error in numberToPos and formatError: offset is longer than source length fix: handle error in numberToPos and formatError Sep 1, 2021
@patak-dev patak-dev merged commit c87763c into vitejs:main Sep 1, 2021
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1-chore Doesn't change code behavior (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants