broccolijs / broccoli-persistent-filter

MIT License
12 stars 33 forks source link

Check file existence before unlinking it #189

Closed gossi closed 4 years ago

gossi commented 4 years ago

Check whether a file exists before unlinking it. Unlinking a non-existing file will throw an error message. See ClarkSource/ember-makeup#242

Dunno if this is the correct way of fixing the original linked issue - yet I also think this is not a wrong way to do so.

gossi commented 4 years ago

A classical This should never have happened moment?

Here is the related filter in question: https://github.com/ClarkSource/ember-makeup/blob/master/ts/plugins/broccoli/extract-schema.ts

I cannot see anything wrong with the filter but I'm personally never against more checks if this enables stability. wdyt?

stefanpenner commented 4 years ago

A classical This should never have happened moment?

Sorta, this is not a mysterious "should never happen moment" rather, when this happens it means we have hit a "this is a legitimately bug moment".

I cannot see anything wrong with the filter but I'm personally never against more checks if this enables

It's a hard balance, but being too defensive can lead to issues, for example the above change adds extra IO which will negative build performance.

If we can explain a rationale scenario where this is required, I am totally on-board with adding it, but reducing build performance to paper over bugs elsewhere is concerning to me.

Although I suspect this unlikely, it is still a possibility that If after understanding the issue, it does become obvious that we need this check or something similar to it. We can take another approach, one without this performance impact. Specifically, we could optimistically unlink, and if unlink throws with the ENOEN error we can gracefully handle the error.


A reproduction would help us understand what the appropriate next steps are...

stefanpenner commented 4 years ago

cc @buschtoens

gossi commented 4 years ago

Idea can be dropped. Gonna close this one.