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: graph supports external and built in modules #130

Merged
merged 3 commits into from
Feb 7, 2022

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Feb 4, 2022

Closes #127

I think it is functionally complete, and I have all the existing tests running. I just need to add tests for the new stuff,

But basically it is a minor refactor for existing code which goes from:

LoadResponse {
  specifier,
  maybe_headers,
  content
}

To:

LoadResponse::Module {
  specifier,
  maybe_headers,
  content
}

And the new external and built-in would be:

LoadResponse::External(specifier);
LoadResponse::BuiltIn(specifier);

They will be represented in the graph as:

ModuleKind::External;
ModuleKind::BuiltIn;

Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

API wise LGTM! I don't think for eszips we need even external specifiers slots in the graph (?) but makes sense for other usecases.

@kitsonk kitsonk changed the title [WIP] feat: graph supports external and built in modules feat: graph supports external and built in modules Feb 7, 2022
@kitsonk kitsonk marked this pull request as ready for review February 7, 2022 01:21
@kitsonk
Copy link
Contributor Author

kitsonk commented Feb 7, 2022

I don't think for eszips we need even external specifiers slots in the graph

deno compile's usage of eszip may require it

@kitsonk kitsonk merged commit f8adbe4 into denoland:main Feb 7, 2022
@kitsonk kitsonk deleted the kitsonk/issue127 branch February 7, 2022 01:25
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.

Allow representation of external and built-in dependencies
2 participants