hasundue / molt

Update dependencies the Deno way
https://jsr.io/@molt
MIT License
79 stars 5 forks source link

fix(core): filter invalid dependencies #166

Closed Milly closed 5 months ago

Milly commented 5 months ago

An assertion error occurs if graph.modules[].dependencies[] has an error data like following:

error: Uncaught (in promise) AssertionError: Expected actual: "undefined" to not be null or undefined.
    at collectFromDependency (file:///work/deno/molt/core/update.ts:260:13)
    at file:///work/deno/molt/core/update.ts:214:16
    ...
{
  "specifier": "@std/collections/filter-keys",
  "code": {
    "error": "JavaScript resolve threw.",
    "span": {
      "start": {"line": 60, "character": 42},
      "end": {"line": 60, "character": 72}
    }
  },
  "isDynamic": true
}

I don't know if it's okay to simply filter. Now it works for me.

hasundue commented 5 months ago

Looks like a case of #146 ? Filtering them out doesn't make those imports updatable but maybe better than throwing.

Can we show warnings on them? Do you think it too annoying?

hasundue commented 5 months ago

Let me fix CI first...

hasundue commented 5 months ago

I believe I can fix #146 in this week so we might just wait for it instead.

Milly commented 5 months ago

Looks like a case of #146?

That's right. Sorry I missed it.

Filtering them out doesn't make those imports updatable but maybe better than throwing. Can we show warnings on them? Do you think it too annoying?

Production code should not generate assertion errors or should be caught properly. This is because the cause of the error is unknown to the user. Of course, it is acceptable to include assert() for developers.

It should be one of the following:

Milly commented 5 months ago

Sorry pushed invalid code, so force pushed...

hasundue commented 5 months ago

Production code should not generate assertion errors or should be caught properly. This is because the cause of the error is unknown to the user. Of course, it is acceptable to include assert() for developers.

Absolutely right. That's why the behavior is labeled as a bug.

Appreciate your additional work, which looks good as a tentative fix. I will probably re-write an equivalent logic in core/graph.ts when I fix #146, but let me merge your change as an emergent patch if you don't mind your PR would be reverted eventually.

hasundue commented 5 months ago

@Milly Can I squash your commits and rewrite the message on merging?