Skip to content

refactor: check assertions per import statement#228

Closed
nayeemrmn wants to merge 34 commits intodenoland:mainfrom
nayeemrmn:import-errors
Closed

refactor: check assertions per import statement#228
nayeemrmn wants to merge 34 commits intodenoland:mainfrom
nayeemrmn:import-errors

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().

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.

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?

@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.

@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