-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 postcss nitpicks #3077
Fix postcss nitpicks #3077
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
7 Ignored Deployments
|
@@ -186,12 +186,11 @@ impl Asset for PostCssTransformedAsset { | |||
) | |||
.await?; | |||
let JavaScriptValue::Value(val) = &*config_value else { | |||
bail!("Expected a value from PostCSS transform"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't want this error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will lead to the same Rust stacktrace leaking out. I figure it's better to use the original CSS to allow the dev server to start up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -191,8 +191,7 @@ | |||
// an external module we don't provide a shim for (e.g. querystring, url). | |||
// For now, we fail semi-silently, but in the future this should be a | |||
// compilation error. | |||
console.error(`Failed to load external module ${id}: ${err}`); | |||
return undefined; | |||
throw new Error(`Failed to load external module ${id}: ${err}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a reason behind that. See comment. Maybe @alexkirsz knows more?
If we remove that we should also remove the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the comment is about providing a better UX during compilation?
As for the runtime UX, it "semi-silently" fails meaning that as soon as you try to use the return value, you lose the real reason for the error happening. I think loudly failing provides a better experience.
🟢 CI successful 🟢Thanks |
Benchmark for d5e0379
Click to view full benchmark
|
Re: vercel/turbo#3065 It also fixes the gnarly stack trace error whenever the node code throws, but it doesn't look very pretty. Not sure how to handle it: <img width="1280" alt="Screen Shot 2022-12-19 at 3 46 56 PM" src="https://user-images.githubusercontent.com/112982/208519095-ec7ad539-5c23-4b95-bcc1-71ba30381e60.png">
Re: vercel/turbo#3065 It also fixes the gnarly stack trace error whenever the node code throws, but it doesn't look very pretty. Not sure how to handle it: <img width="1280" alt="Screen Shot 2022-12-19 at 3 46 56 PM" src="https://user-images.githubusercontent.com/112982/208519095-ec7ad539-5c23-4b95-bcc1-71ba30381e60.png">
Re: #3065
It also fixes the gnarly stack trace error whenever the node code throws, but it doesn't look very pretty. Not sure how to handle it: