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

Validate error path for addDependency() (#197) #199

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

wabain
Copy link
Contributor

@wabain wabain commented Jan 15, 2022

Fixes #197, which is caused by undefined being passed to the loader addDependency method when handling errors. A Svelte CompileError doesn't have a file property, but it does have filename, so try either of those and call addDependency with the value if it is a string (and not equal to the path of the top-level resource being loaded).

In addition to updating tests, I've checked manually that error reporting and rebuilding after an error works in a webpack project that had been hitting ERR_INVALID_ARG_TYPE before this patch.

index.js Outdated
@@ -78,7 +78,10 @@ module.exports = function(source, map) {

callback(null, js.code, js.map);
}, err => {
this.addDependency(err.file);
const file = err.filename || err.file;
Copy link

Choose a reason for hiding this comment

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

this statement favors the err.filename value, is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't accidental, but it is arbitrary. I don't have a reason to favor either over the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revisiting this, it occurs to me that one obvious reason to prefer err.file is that it maintains the status quo if there's an error value out there which has both properties. I'll push a change to switch the preference.

Copy link

Choose a reason for hiding this comment

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

https://github.com/sveltejs/svelte/blob/dafbdc286eef3de2243088a9a826e6899e20465c/src/compiler/utils/error.ts#L9
The property of CompileError is filename, not file. Checking is good, but I don't think file should be cared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's been a while now, but I think I believed other error types were reachable here.

@basuke
Copy link

basuke commented May 2, 2022

Why isn't this PR merged in? The crash is very annoying. I appreciate this merging-in pretty soon.

@pngwn
Copy link
Member

pngwn commented May 2, 2022

Your potential appreciation of any 'pretty-soon' merging is duly noted. Thank you for informing us.

@basuke
Copy link

basuke commented May 2, 2022

@pngwn I'm sorry if my comment made you feeling bad but I really do not get the reason why this PR is halting. Are there any issue or something unclear?

FYI: the crash won't happens when I don't use <script lang=ts>. The compilation error will be caught by the other error handler:

svelte-loader/index.js

Lines 87 to 89 in 79111a9

// wrap error to provide correct
// context when logging to console
callback(new Error(`${err.name}: ${err.toString()}`));

@pngwn
Copy link
Member

pngwn commented May 2, 2022

I'm sorry if my comment made you feeling bad but I really do not get the reason why this PR is halting. Are there any issue or something unclear?

No other reason that we are very busy. We'll get around to it as soon as we can.

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.

syntax error in svelte component causes entire webpack watch process to die
5 participants