Skip to content

Commit 91ec8d3

Browse files
Devon GovettDevon Govett
Devon Govett
authored and
Devon Govett
committed
Propagate error location info to JS in bundler
Fixes parcel-bundler#507
1 parent a36c105 commit 91ec8d3

File tree

3 files changed

+178
-78
lines changed

3 files changed

+178
-78
lines changed

‎node/src/lib.rs‎

Lines changed: 73 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ use lightningcss::stylesheet::{
1212
};
1313
use lightningcss::targets::{Browsers, Features, Targets};
1414
use lightningcss::visitor::Visit;
15+
use napi::bindgen_prelude::{FromNapiValue, ToNapiValue};
1516
use parcel_sourcemap::SourceMap;
1617
use serde::{Deserialize, Serialize};
1718
use std::collections::{HashMap, HashSet};
@@ -83,7 +84,7 @@ fn transform(ctx: CallContext) -> napi::Result<JsUnknown> {
8384

8485
match res {
8586
Ok(res) => res.into_js(*ctx.env),
86-
Err(err) => err.throw(*ctx.env, Some(code)),
87+
Err(err) => Err(err.into_js_error(*ctx.env, Some(code))?),
8788
}
8889
}
8990

@@ -104,7 +105,7 @@ fn transform_style_attribute(ctx: CallContext) -> napi::Result<JsUnknown> {
104105

105106
match res {
106107
Ok(res) => res.into_js(ctx),
107-
Err(err) => err.throw(*ctx.env, Some(code)),
108+
Err(err) => Err(err.into_js_error(*ctx.env, Some(code))?),
108109
}
109110
}
110111

@@ -146,28 +147,7 @@ mod bundle {
146147

147148
match res {
148149
Ok(res) => res.into_js(*ctx.env),
149-
Err(err) => {
150-
let code = match &err {
151-
CompileError::ParseError(Error {
152-
loc: Some(ErrorLocation { filename, .. }),
153-
..
154-
})
155-
| CompileError::PrinterError(Error {
156-
loc: Some(ErrorLocation { filename, .. }),
157-
..
158-
})
159-
| CompileError::MinifyError(Error {
160-
loc: Some(ErrorLocation { filename, .. }),
161-
..
162-
})
163-
| CompileError::BundleError(Error {
164-
loc: Some(ErrorLocation { filename, .. }),
165-
..
166-
}) => Some(fs.read(Path::new(filename))?),
167-
_ => None,
168-
};
169-
err.throw(*ctx.env, code)
170-
}
150+
Err(err) => Err(err.into_js_error(*ctx.env, None)?),
171151
}
172152
}
173153

@@ -270,8 +250,8 @@ mod bundle {
270250
ctx.env.get_undefined()
271251
})?;
272252
let eb = env.create_function_from_closure("error_callback", move |ctx| {
273-
// TODO: need a way to convert a JsUnknown to an Error
274-
tx2.send(Err(napi::Error::from_reason("Promise rejected"))).unwrap();
253+
let res = ctx.get::<JsUnknown>(0)?;
254+
tx2.send(Err(napi::Error::from(res))).unwrap();
275255
ctx.env.get_undefined()
276256
})?;
277257
then.call(Some(&result), &[cb, eb])?;
@@ -379,7 +359,10 @@ mod bundle {
379359
config: BundleConfig,
380360
visitor: Option<JsVisitor>,
381361
env: Env,
382-
) -> napi::Result<JsObject> {
362+
) -> napi::Result<JsObject>
363+
where
364+
P::Error: IntoJsError,
365+
{
383366
let (deferred, promise) = env.create_deferred()?;
384367

385368
let tsfn = if let Some(mut visitor) = visitor {
@@ -402,7 +385,7 @@ mod bundle {
402385

403386
// Run bundling task in rayon threadpool.
404387
rayon::spawn(move || {
405-
match compile_bundle(
388+
let res = compile_bundle(
406389
unsafe { std::mem::transmute::<&'_ P, &'static P>(&provider) },
407390
&config,
408391
tsfn.map(move |tsfn| {
@@ -425,10 +408,12 @@ mod bundle {
425408
})
426409
}
427410
}),
428-
) {
429-
Ok(v) => deferred.resolve(|env| v.into_js(env)),
430-
Err(err) => deferred.reject(err.into()),
431-
}
411+
);
412+
413+
deferred.resolve(move |env| match res {
414+
Ok(v) => v.into_js(env),
415+
Err(err) => Err(err.into_js_error(env, None)?),
416+
});
432417
});
433418

434419
Ok(promise)
@@ -600,7 +585,7 @@ fn compile<'i>(
600585
code: &'i str,
601586
config: &Config,
602587
visitor: &mut Option<JsVisitor>,
603-
) -> Result<TransformResult<'i>, CompileError<'i, std::io::Error>> {
588+
) -> Result<TransformResult<'i>, CompileError<'i, napi::Error>> {
604589
let drafts = config.drafts.as_ref();
605590
let non_standard = config.non_standard.as_ref();
606591
let warnings = Some(Arc::new(RwLock::new(Vec::new())));
@@ -891,7 +876,7 @@ fn compile_attr<'i>(
891876
code: &'i str,
892877
config: &AttrConfig,
893878
visitor: &mut Option<JsVisitor>,
894-
) -> Result<AttrResult<'i>, CompileError<'i, std::io::Error>> {
879+
) -> Result<AttrResult<'i>, CompileError<'i, napi::Error>> {
895880
let warnings = if config.error_recovery {
896881
Some(Arc::new(RwLock::new(Vec::new())))
897882
} else {
@@ -975,8 +960,8 @@ impl<'i, E: std::error::Error> std::fmt::Display for CompileError<'i, E> {
975960
}
976961
}
977962

978-
impl<'i, E: std::error::Error> CompileError<'i, E> {
979-
fn throw(self, env: Env, code: Option<&str>) -> napi::Result<JsUnknown> {
963+
impl<'i, E: IntoJsError + std::error::Error> CompileError<'i, E> {
964+
fn into_js_error(self, env: Env, code: Option<&str>) -> napi::Result<napi::Error> {
980965
let reason = self.to_string();
981966
let data = match &self {
982967
CompileError::ParseError(Error { kind, .. }) => env.to_js_value(kind)?,
@@ -986,38 +971,71 @@ impl<'i, E: std::error::Error> CompileError<'i, E> {
986971
_ => env.get_null()?.into_unknown(),
987972
};
988973

989-
match self {
974+
let (js_error, loc) = match self {
975+
CompileError::BundleError(Error {
976+
loc,
977+
kind: BundleErrorKind::ResolverError(e),
978+
}) => {
979+
// Add location info to existing JS error if available.
980+
(e.into_js_error(env)?, loc)
981+
}
990982
CompileError::ParseError(Error { loc, .. })
991983
| CompileError::PrinterError(Error { loc, .. })
992984
| CompileError::MinifyError(Error { loc, .. })
993985
| CompileError::BundleError(Error { loc, .. }) => {
994986
// Generate an error with location information.
995987
let syntax_error = env.get_global()?.get_named_property::<napi::JsFunction>("SyntaxError")?;
996988
let reason = env.create_string_from_std(reason)?;
997-
let mut obj = syntax_error.new_instance(&[reason])?;
998-
if let Some(loc) = loc {
999-
let line = env.create_int32((loc.line + 1) as i32)?;
1000-
let col = env.create_int32(loc.column as i32)?;
1001-
let filename = env.create_string_from_std(loc.filename)?;
1002-
obj.set_named_property("fileName", filename)?;
1003-
if let Some(code) = code {
1004-
let source = env.create_string(code)?;
1005-
obj.set_named_property("source", source)?;
1006-
}
1007-
let mut loc = env.create_object()?;
1008-
loc.set_named_property("line", line)?;
1009-
loc.set_named_property("column", col)?;
1010-
obj.set_named_property("loc", loc)?;
989+
let obj = syntax_error.new_instance(&[reason])?;
990+
(obj.into_unknown(), loc)
991+
}
992+
_ => return Ok(self.into()),
993+
};
994+
995+
if js_error.get_type()? == napi::ValueType::Object {
996+
let mut obj: JsObject = unsafe { js_error.cast() };
997+
if let Some(loc) = loc {
998+
let line = env.create_int32((loc.line + 1) as i32)?;
999+
let col = env.create_int32(loc.column as i32)?;
1000+
let filename = env.create_string_from_std(loc.filename)?;
1001+
obj.set_named_property("fileName", filename)?;
1002+
if let Some(code) = code {
1003+
let source = env.create_string(code)?;
1004+
obj.set_named_property("source", source)?;
10111005
}
1012-
obj.set_named_property("data", data)?;
1013-
env.throw(obj)?;
1014-
Ok(env.get_undefined()?.into_unknown())
1006+
let mut loc = env.create_object()?;
1007+
loc.set_named_property("line", line)?;
1008+
loc.set_named_property("column", col)?;
1009+
obj.set_named_property("loc", loc)?;
10151010
}
1016-
_ => Err(self.into()),
1011+
obj.set_named_property("data", data)?;
1012+
Ok(obj.into_unknown().into())
1013+
} else {
1014+
Ok(js_error.into())
10171015
}
10181016
}
10191017
}
10201018

1019+
trait IntoJsError {
1020+
fn into_js_error(self, env: Env) -> napi::Result<JsUnknown>;
1021+
}
1022+
1023+
impl IntoJsError for std::io::Error {
1024+
fn into_js_error(self, env: Env) -> napi::Result<JsUnknown> {
1025+
let reason = self.to_string();
1026+
let syntax_error = env.get_global()?.get_named_property::<napi::JsFunction>("SyntaxError")?;
1027+
let reason = env.create_string_from_std(reason)?;
1028+
let obj = syntax_error.new_instance(&[reason])?;
1029+
Ok(obj.into_unknown())
1030+
}
1031+
}
1032+
1033+
impl IntoJsError for napi::Error {
1034+
fn into_js_error(self, env: Env) -> napi::Result<JsUnknown> {
1035+
unsafe { JsUnknown::from_napi_value(env.raw(), ToNapiValue::to_napi_value(env.raw(), self)?) }
1036+
}
1037+
}
1038+
10211039
impl<'i, E: std::error::Error> From<Error<ParserError<'i>>> for CompileError<'i, E> {
10221040
fn from(e: Error<ParserError<'i>>) -> CompileError<'i, E> {
10231041
CompileError::ParseError(e)

‎node/test/bundle.test.mjs‎

Lines changed: 99 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -181,10 +181,8 @@ test('read throw', async () => {
181181
}
182182

183183
if (!error) throw new Error(`\`testReadThrow()\` failed. Expected \`bundleAsync()\` to throw, but it did not.`);
184-
// TODO: need support for napi-rs to propagate errors.
185-
// if (!error.message.includes(`\`read()\` threw error:`) || !error.message.includes(`Oh noes! Failed to read \`foo.css\`.`)) {
186-
// throw new Error(`\`testReadThrow()\` failed. Expected \`bundleAsync()\` to throw a specific error message, but it threw a different error:\n${error.message}`);
187-
// }
184+
assert.equal(error.message, `Oh noes! Failed to read \`foo.css\`.`);
185+
assert.equal(error.loc, undefined); // error occurred when reading initial file, no location info available.
188186
});
189187

190188
test('async read throw', async () => {
@@ -203,10 +201,62 @@ test('async read throw', async () => {
203201
}
204202

205203
if (!error) throw new Error(`\`testReadThrow()\` failed. Expected \`bundleAsync()\` to throw, but it did not.`);
206-
// TODO: need support for napi-rs to propagate errors.
207-
// if (!error.message.includes(`\`read()\` threw error:`) || !error.message.includes(`Oh noes! Failed to read \`foo.css\`.`)) {
208-
// throw new Error(`\`testReadThrow()\` failed. Expected \`bundleAsync()\` to throw a specific error message, but it threw a different error:\n${error.message}`);
209-
// }
204+
assert.equal(error.message, `Oh noes! Failed to read \`foo.css\`.`);
205+
assert.equal(error.loc, undefined); // error occurred when reading initial file, no location info available.
206+
});
207+
208+
test('read throw with location info', async () => {
209+
let error = undefined;
210+
try {
211+
await bundleAsync({
212+
filename: 'foo.css',
213+
resolver: {
214+
read(file) {
215+
if (file === 'foo.css') {
216+
return '@import "bar.css"';
217+
}
218+
throw new Error(`Oh noes! Failed to read \`${file}\`.`);
219+
}
220+
},
221+
});
222+
} catch (err) {
223+
error = err;
224+
}
225+
226+
if (!error) throw new Error(`\`testReadThrow()\` failed. Expected \`bundleAsync()\` to throw, but it did not.`);
227+
assert.equal(error.message, `Oh noes! Failed to read \`bar.css\`.`);
228+
assert.equal(error.fileName, 'foo.css');
229+
assert.equal(error.loc, {
230+
line: 1,
231+
column: 1
232+
});
233+
});
234+
235+
test('async read throw with location info', async () => {
236+
let error = undefined;
237+
try {
238+
await bundleAsync({
239+
filename: 'foo.css',
240+
resolver: {
241+
async read(file) {
242+
if (file === 'foo.css') {
243+
return '@import "bar.css"';
244+
}
245+
throw new Error(`Oh noes! Failed to read \`${file}\`.`);
246+
}
247+
},
248+
});
249+
} catch (err) {
250+
error = err;
251+
}
252+
253+
if (!error) throw new Error(`\`testReadThrow()\` failed. Expected \`bundleAsync()\` to throw, but it did not.`);
254+
assert.equal(error.message, `Oh noes! Failed to read \`bar.css\`.`);
255+
assert.equal(error.fileName, 'foo.css');
256+
assert.equal(error.loc, {
257+
line: 1,
258+
column: 1
259+
});
210260
});
211261

212262
test('resolve throw', async () => {
@@ -225,10 +275,12 @@ test('resolve throw', async () => {
225275
}
226276

227277
if (!error) throw new Error(`\`testResolveThrow()\` failed. Expected \`bundleAsync()\` to throw, but it did not.`);
228-
// TODO: need support for napi-rs to propagate errors.
229-
// if (!error.message.includes(`\`resolve()\` threw error:`) || !error.message.includes(`Oh noes! Failed to resolve \`root:hello/world.css\` from \`tests/testdata/css/foo.css\`.`)) {
230-
// throw new Error(`\`testResolveThrow()\` failed. Expected \`bundleAsync()\` to throw a specific error message, but it threw a different error:\n${error.message}`);
231-
// }
278+
assert.equal(error.message, `Oh noes! Failed to resolve \`root:hello/world.css\` from \`tests/testdata/foo.css\`.`);
279+
assert.equal(error.fileName, 'tests/testdata/foo.css');
280+
assert.equal(error.loc, {
281+
line: 1,
282+
column: 1
283+
});
232284
});
233285

234286
test('async resolve throw', async () => {
@@ -247,10 +299,12 @@ test('async resolve throw', async () => {
247299
}
248300

249301
if (!error) throw new Error(`\`testResolveThrow()\` failed. Expected \`bundleAsync()\` to throw, but it did not.`);
250-
// TODO: need support for napi-rs to propagate errors.
251-
// if (!error.message.includes(`\`resolve()\` threw error:`) || !error.message.includes(`Oh noes! Failed to resolve \`root:hello/world.css\` from \`tests/testdata/css/foo.css\`.`)) {
252-
// throw new Error(`\`testResolveThrow()\` failed. Expected \`bundleAsync()\` to throw a specific error message, but it threw a different error:\n${error.message}`);
253-
// }
302+
assert.equal(error.message, `Oh noes! Failed to resolve \`root:hello/world.css\` from \`tests/testdata/foo.css\`.`);
303+
assert.equal(error.fileName, 'tests/testdata/foo.css');
304+
assert.equal(error.loc, {
305+
line: 1,
306+
column: 1
307+
});
254308
});
255309

256310
test('read return non-string', async () => {
@@ -269,9 +323,7 @@ test('read return non-string', async () => {
269323
}
270324

271325
if (!error) throw new Error(`\`testReadReturnNonString()\` failed. Expected \`bundleAsync()\` to throw, but it did not.`);
272-
if (!error.message.includes(`InvalidArg, expect String, got: Number`)) {
273-
throw new Error(`\`testReadReturnNonString()\` failed. Expected \`bundleAsync()\` to throw a specific error message, but it threw a different error:\n${error.message}`);
274-
}
326+
assert.equal(error.message, 'expect String, got: Number');
275327
});
276328

277329
test('resolve return non-string', async () => {
@@ -290,9 +342,35 @@ test('resolve return non-string', async () => {
290342
}
291343

292344
if (!error) throw new Error(`\`testResolveReturnNonString()\` failed. Expected \`bundleAsync()\` to throw, but it did not.`);
293-
if (!error.message.includes(`InvalidArg, expect String, got: Number`)) {
294-
throw new Error(`\`testResolveReturnNonString()\` failed. Expected \`bundleAsync()\` to throw a specific error message, but it threw a different error:\n${error.message}`);
345+
assert.equal(error.message, 'expect String, got: Number');
346+
assert.equal(error.fileName, 'tests/testdata/foo.css');
347+
assert.equal(error.loc, {
348+
line: 1,
349+
column: 1
350+
});
351+
});
352+
353+
test('should throw with location info on syntax errors', async () => {
354+
let error = undefined;
355+
try {
356+
await bundleAsync({
357+
filename: 'tests/testdata/foo.css',
358+
resolver: {
359+
read() {
360+
return '.foo'
361+
}
362+
},
363+
});
364+
} catch (err) {
365+
error = err;
295366
}
367+
368+
assert.equal(error.message, `Unexpected end of input`);
369+
assert.equal(error.fileName, 'tests/testdata/foo.css');
370+
assert.equal(error.loc, {
371+
line: 1,
372+
column: 5
373+
});
296374
});
297375

298376
test.run();

0 commit comments

Comments
 (0)