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

refactor: check assertions per import statement #228

Closed
wants to merge 34 commits into from

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented Feb 12, 2023

Closes #132
Fixes #160
Supersedes #155

Assertions appropriately no longer influence module loading/parsing. Instead they are checked in a pass at the end; resulting errors are stored in Import::errors: Vec<ImportError>. 'Import errors' are a third kind of error alongside module slot errors and resolution errors which are now checked in ModuleEntryIterator::validate().

src/graph.rs Show resolved Hide resolved
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

I'll review the rest of it later.

Cargo.toml Outdated Show resolved Hide resolved
src/analyzer.rs Outdated Show resolved Hide resolved
src/analyzer.rs Show resolved Hide resolved
src/analyzer.rs Outdated Show resolved Hide resolved
src/analyzer.rs Outdated Show resolved Hide resolved
src/analyzer.rs Outdated Show resolved Hide resolved
src/analyzer.rs Outdated Show resolved Hide resolved
src/graph.rs Outdated Show resolved Hide resolved
src/graph.rs Outdated Show resolved Hide resolved
src/graph.rs Outdated Show resolved Hide resolved
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

Thanks for this @nayeemrmn. It's looking better than main, but I'm not sure about the name "imports". Perhaps this should be something more generic because it also includes exports and typescript path references?

test.ts Outdated Show resolved Hide resolved
src/graph.rs Show resolved Hide resolved
src/graph.rs Outdated Show resolved Hide resolved
src/graph.rs Outdated Show resolved Hide resolved
src/graph.rs Outdated Show resolved Hide resolved
src/graph.rs Outdated Show resolved Hide resolved
@nayeemrmn
Copy link
Collaborator Author

Regarding 2.0 breakages, seems like the only breakage is Dependency::assertionType. Could we keep that as legacy for a month to unblock this PR?

@dsherret
Copy link
Member

Probably not because merging this would also require more work upgrading the rest of the code in other repos and that would block progress on getting npm specifiers in deno_graph, which is the priority at the moment because we want to get them in a more ideal state for integration with Deploy and other tooling. It's just waiting a couple weeks until early March.

src/graph.rs Outdated Show resolved Hide resolved
@nayeemrmn
Copy link
Collaborator Author

@dsherret I went ahead and preserved assertionType in the serialization for legacy. It's a very self-contained, small addition in serialize_dependencies(). This can be considered for landing without breaking deno info now.

@nayeemrmn
Copy link
Collaborator Author

With import assertions becoming import attributes and them being part of the cache key, this PR goes in the wrong direction for the future. Closing.

@nayeemrmn nayeemrmn closed this Jul 5, 2023
@nayeemrmn nayeemrmn deleted the import-errors branch February 14, 2024 18:31
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.

Don't enforce assertions on type-only imports Support multiple ranges for an import in the graph
2 participants