Skip to content
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

feat: add ModuleGraphError::TypesResolutionError #311

Conversation

dsherret
Copy link
Member

Adds a new TypesResolutionError so that it's easier to identify which resolution errors are for type checking or not and enhance the error messages.

FYI @nayeemrmn. For some reason I can't add you as a reviewer.

@dsherret dsherret requested a review from bartlomieju October 24, 2023 18:01
Result<&'a Module, &'a ModuleGraphError>,
);
type ModuleResult<'a> =
(&'a ModuleSpecifier, Result<&'a Module, &'a ModuleError>);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really make sense to store resolution errors in the module slots, so changed this from ModuleGraphError to just ModuleError.

@bartlomieju
Copy link
Member

FYI @nayeemrmn. For some reason I can't add you as a reviewer.

I added Nayeem to this repo. This should be fixed now.

@dsherret dsherret requested a review from nayeemrmn October 24, 2023 18:25
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I like this change, does it mean that we'll be able to not fail on wrong types specifier but still fail if the actual code can't be resolved?

Copy link
Collaborator

@nayeemrmn nayeemrmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Does it offer different functionality from just checking the type-only status of the graph branch when traversing? Feels like it should be a lot simpler to identify type-only graph arcs with #272

@dsherret
Copy link
Member Author

LGTM, I like this change, does it mean that we'll be able to not fail on wrong types specifier but still fail if the actual code can't be resolved?

That was already possible before by using graph.walk(... { follow_types: false, .. }).errors(), which is what graph.valid()? seems to use under the hood.

Does it offer different functionality from just checking the type-only status of the graph branch when traversing?

@nayeemrmn I don't understand. Could you clarify?

Feels like it should be a lot simpler to identify type-only graph arcs with #272

I'm unsure if we should make that change anymore. It's now possible to have specifiers like "./a" resolve to a.js for code and a.d.ts for types

@nayeemrmn
Copy link
Collaborator

Does it offer different functionality from just checking the type-only status of the graph branch when traversing?

@nayeemrmn I don't understand. Could you clarify?

Nevermind, the ModuleGraphError enum is meant for when you're getting e.g. a list of errors from a graph without context, so you won't have the type-only traversal context I mentioned. It makes sense there.

Feels like it should be a lot simpler to identify type-only graph arcs with #272

I'm unsure if we should make that change anymore. It's now possible to have specifiers like "./a" resolve to a.js for code and a.d.ts for types

Yeah.. I'll see what to do with that PR. I still think we can get a flatter structure but will have to rethink it.

@dsherret dsherret merged commit cf526b8 into denoland:main Oct 24, 2023
@dsherret dsherret deleted the feat_add_module_graph_error_types_resolution_error branch October 24, 2023 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants