stealjs / steal

Gets JavaScript
https://stealjs.com
MIT License
1.37k stars 522 forks source link

Steal errors contain a cycle, breaking Feathers error handling #1509

Closed nlundquist closed 5 years ago

nlundquist commented 5 years ago

If Steal produces an error during an SSR render, Feathers will try to serialize it as part of it's error handling, this will fail since the Steal error contains a cycle. This effectively blocks the error you need to know about behind a useless serialization error.

This Feathers serialization occurs here: https://github.com/feathersjs/feathers/blob/4e4d10a4531db1b69ee1ea7e0df8c199a98b6f49/packages/errors/lib/index.js#L28 after being called here: https://github.com/feathersjs/feathers/blob/1c5c0d3539b1ea04d523e9d3137e2d681f45e0f4/packages/express/lib/error-handler.js#L40-L42

The error being serialized (produced by Steal) includes a cycle shaped like the following:

thisError = {
    message: ...
    errors: [ thisError ]
    ...
}

This is easily reproduced in @MarcGoddard's test app: https://github.com/MarcGodard/site-template

  1. Install the dependencies of the app above
  2. Remove "moment" from client/node_modules
  3. Run mongodb server as per readme
  4. Run app via "npm run dev"
  5. Load "localhost:8080"

At this point the app will try to SSR render the page, failing due to the missing moment dependency, but the error printed will be a JSON serialization failure due to cycle in the underlying Steal error.

matthewp commented 5 years ago

Do you know where the error in Steal is produced? We could maybe implement toJSON() or something...

MarcGodard commented 5 years ago

Thanks @nlundquist

If anyone needs access to the private repo mentioned, let me know.

m-mujica commented 5 years ago

closing this one since steal is not adding the cycle.

MarcGodard commented 5 years ago

@m-mujica could you help me with suggesting what should be my next step? I think the problem is in can-connect-feathers because it includes the feathers-errors package which no longer is used.

m-mujica commented 5 years ago

@MarcGodard I moved this issue to can-zone where the bug has to be fixed. I sent a PR https://github.com/canjs/can-zone/pull/194.. Once I figure out what's breaking the build, we'll merge and release it. Once it's out I'll let you know so you can test it in your repo.