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

Remove source phase resolution time negative error types #4194

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Aug 13, 2024

The source phase import binding name tests can be tested more easily with the resolution time negative error types removed, as they are host defined errors.

Refs: #4121 (comment)

@legendecas legendecas requested a review from a team as a code owner August 13, 2024 23:38

import source from '<do not resolve>';
import from from '<do not resolve>';
import source from '<module source>';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should avoid using <module source> in non-source imports.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Aug 14, 2024

Instead of doing this, can we just remove type: SyntaxError? We just need to assert that we are getting an error at resolution time, regardless or where it comes from, since we are just checking that it does not throw when parsing.

This is to avoid having to require another flag, which may prevent some implementations from running the test.

@legendecas legendecas force-pushed the module-source-specifier branch from 4913f78 to 3756e28 Compare January 3, 2025 15:49
@legendecas legendecas changed the title Use <module source> specifier Remove source phase resolution time negative error types Jan 3, 2025
@legendecas
Copy link
Member Author

Instead of doing this, can we just remove type: SyntaxError?

Updated. Thanks for suggestion, sounds like a good alternative to me.

@legendecas legendecas force-pushed the module-source-specifier branch from 3756e28 to 978e07f Compare January 6, 2025 13:40
@Ms2ger Ms2ger requested a review from nicolo-ribaudo January 7, 2025 14:47
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Looks good, if Error is something valid to write there to mean "any error"

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.

4 participants