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: use loc.file from rollup errors if available #19222

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

karlvr
Copy link

@karlvr karlvr commented Jan 17, 2025

Description

Currently, Rollup errors output a filename that is actually the module name rather than the file where the error occurred. In the case of less, which supports imports, this can mean that the incorrect filename is output. Also note that the line and column numbers output are, in this case, not from the outputted file (confusing!).

This PR adds detection of loc.file and uses that in preference to the module id (although I've also included it for completeness).

Example

Here is an example of the error message prior to this PR. The file identified is the module / entrypoint rather than the actual file:

[vite:css] [less] Could not parse call arguments or missing ')'
file: /src/web/src/main/frontend/less/screen.less:257:51
    at process (file:///src/web/src/main/frontend/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected]/node_modules/vite/dist/node/chunks/dep-BJP6rrE_.js:49569:33)
    at async compileCSSPreprocessors (file:///src/web/src/main/frontend/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected]/node_modules/vite/dist/node/chunks/dep-BJP6rrE_.js:48437:28)
    at async compileCSS (file:///src/web/src/main/frontend/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected]/node_modules/vite/dist/node/chunks/dep-BJP6rrE_.js:48495:32)
    at async Object.transform (file:///src/web/src/main/frontend/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected]/node_modules/vite/dist/node/chunks/dep-BJP6rrE_.js:47862:11)
    at async transform (file:///src/web/src/main/frontend/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/node-entry.js:19787:16)
    at async ModuleLoader.addModuleSource (file:///src/web/src/main/frontend/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/node-entry.js:20004:23)

Here is an example of the error message with this PR, note that the correct filename is identified:

[vite:css] [less] Could not parse call arguments or missing ')'
file: /src/web/src/main/frontend/less/components/site-header.less:257:51 (/src/web/src/main/frontend/less/screen.less)
    at process (file:///src/web/src/main/frontend/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected]/node_modules/vite/dist/node/chunks/dep-BJP6rrE_.js:49569:33)
    at async compileCSSPreprocessors (file:///src/web/src/main/frontend/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected]/node_modules/vite/dist/node/chunks/dep-BJP6rrE_.js:48437:28)
    at async compileCSS (file:///src/web/src/main/frontend/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected]/node_modules/vite/dist/node/chunks/dep-BJP6rrE_.js:48495:32)
    at async Object.transform (file:///src/web/src/main/frontend/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected]/node_modules/vite/dist/node/chunks/dep-BJP6rrE_.js:47862:11)
    at async transform (file:///src/web/src/main/frontend/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/node-entry.js:19787:16)
    at async ModuleLoader.addModuleSource (file:///src/web/src/main/frontend/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/node-entry.js:20004:23)

@karlvr karlvr force-pushed the improve-rollup-errors branch from 6c5fd7f to 43b5043 Compare January 17, 2025 22:15
@karlvr karlvr changed the title Use loc.file from rollup errors if available fix: Use loc.file from rollup errors if available Jan 19, 2025
The loc.file references the file where the error occurred (containing the displayed line and column), rather than e.id which is the module.
@karlvr karlvr force-pushed the improve-rollup-errors branch from 43b5043 to 709c291 Compare January 19, 2025 22:55
@karlvr karlvr changed the title fix: Use loc.file from rollup errors if available fix: use loc.file from rollup errors if available Jan 19, 2025
Copy link
Collaborator

@hi-ogawa hi-ogawa left a comment

Choose a reason for hiding this comment

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

Makes sense 👍

It looks like error overlay simply does err.loc?.file || err.id, but showing both seems nice too.

const [file] = (err.loc?.file || err.id || 'unknown file').split(`?`)
if (err.loc) {
this.text('.file', `${file}:${err.loc.line}:${err.loc.column}`, links)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants