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

Add Importer.invert_non_canonical_scheme option to embedded protocol #3905

Closed
wants to merge 1 commit into from

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Jul 27, 2024

This is part of sass/embedded-host-node#309. In order for the legacy importer to properly load schemed url as path and get the containing url when doing so, we need to set legacy-importer-file as the only canonical scheme, and everything else as non-canonical.

This PR attempts to achieve that with minimum change, that is to add an invert_non_canonical_scheme flag, which would invert the meaning of non_canonical_scheme set, and treat the set as explicit canonical_scheme. Because this option would be confusing to the end-users, there is no plan to add this to the JS-API that it will be used exclusively for the embedded host to emulate legacy API.

@ntkme ntkme force-pushed the invert_non_canonical_scheme branch from 734911b to 10f1e08 Compare July 27, 2024 18:28
@nex3
Copy link
Contributor

nex3 commented Jul 31, 2024

If we're going to do this, I'd like to have a compelling use-case beyond just cleaning up the legacy API. Legacy API cleanups, as valuable as they may be, are going to be irrelevant in six months or sooner. We've been working with frameworks to get better support for the modern API, and we're going to remove the legacy API as well in 2.0.0 which is coming soon. But protocol changes will last much longer than that, so I want to be more careful with them.

@ntkme
Copy link
Contributor Author

ntkme commented Aug 1, 2024

While the concerns around 2.0 is completely fair, there is a gap between the current design of the Dart API vs JS API:

  • In Dart API isNonCanonicalScheme is a flexible method that user can override and implement different strategies. For example: allowlist, blocklist, pattern matching, etc.
  • In JS API nonCanonicalScheme is strictly a blocklist, which limits what the user can do - For example, in this particular case what I wanted is an allowlist instead of a blocklist.

I understand that the JS API design with only blocklist is mainly to avoid round trips between embedded host and compiler for determining if schemes are canonical or not. However, only having a blocklist option might be too limited. So, I do see some value adding this, or perhaps even consider changing the JS API in terms of how can we enhance its capability to get closer to Dart API's isNonCanonicalScheme without involving round trips.

@nex3
Copy link
Contributor

nex3 commented Aug 1, 2024

You're right that there is a gap, I just don't know how likely the gap is to actually matter in terms of utility for users. I think generally an importer that allows any scheme to be non-canonical is likely to be confusing to users, and I'd rather not support it without a specific use-case.

@nex3
Copy link
Contributor

nex3 commented Aug 26, 2024

I think I'm going to close this out along with sass/embedded-host-node#309 and sass/dart-sass#2291. I'm not sold on this having a strong use-case, and I think the existing legacy importer is generally good enough—its primary goal is to support migration of users to Dart Sass, which it's been successful at, and we'll be getting rid of it soon anyway.

@nex3 nex3 closed this Aug 26, 2024
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.

2 participants