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

Parse Deprecation.forVersion on compiler side #3868

Merged
merged 1 commit into from
May 30, 2024

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented May 18, 2024

This allow the embedded-host-ruby to accept a deprecation version as input, without the need of maintaining the complete deprecation list on the host side.

@jathak
Copy link
Member

jathak commented May 20, 2024

A couple of things related to this:

  1. I'm currently working on some PRs that will add a single-source-of-truth for the list of deprecations that will be a YAML file in this repo. Dart Sass and the Node embedded host will generate their list of deprecations based on it. Would that make sense for the Ruby embedded host too? If so, will this PR then be necessary?

  2. If we do make this change, it will require a language proposal since it changes the embedded protocol, though the fast-track process should be sufficient.

  3. If we do include versions as part of the embedded protocol, then it should only apply to fatal_deprecation (matching the CLI and JS API), as mass-silencing deprecations is not something we want to allow and future deprecations don't have versions yet, so passing versions to the other two flags doesn't make sense.

@ntkme
Copy link
Contributor Author

ntkme commented May 20, 2024

I'm currently working on some PRs that will add a single-source-of-truth for the list of deprecations that will be a YAML file in this repo. Dart Sass and the Node embedded host will generate their list of deprecations based on it. Would that make sense for the Ruby embedded host too? If so, will this PR then be necessary?

In theory yes, but at the same time I prefer more logic to be handled on the compiler side, to reduce duplicated code logic, and potential inconsistency due to errors in different implementations.

@ntkme
Copy link
Contributor Author

ntkme commented May 21, 2024

If we do include versions as part of the embedded protocol, then it should only apply to fatal_deprecation (matching the CLI and JS API), as mass-silencing deprecations is not something we want to allow and future deprecations don't have versions yet, so passing versions to the other two flags doesn't make sense.

I have update the PRs to match the CLI behavior. For JS API in embedded-host-node, unfortunately this is only enforced when users write code in TypeScript. When writing in pure JavaScript I can do the following to silence the "slash-div" with a version input, and it indeed silences the warning due to lack of runtime checks:

sass.compileString('a {b: (1px / 2);}', {silenceDeprecations: [sass.Version.parse('2.0.0')]})

@jathak If this should have not worked as you have described, implementing this on the compiler side can help host implementations to have better consistency.

@johnfairh
Copy link
Contributor

I implemented the deprecations API for my host & fwiw, before looking at the details, did expect the by-version flavour to be implemented on the compiler side. The approach proposed here makes naive sense to me as providing a single implementation of the mapping.

Might be useful to hear the reasoning/benefits for the previous proposal where each host individually implements the mapping based on the yaml file?

@ntkme
Copy link
Contributor Author

ntkme commented May 21, 2024

Might be useful to hear the reasoning/benefits for the previous proposal where each host individually implements the mapping based on the yaml file?

The yaml file is to provide host implementors a source to generate static constants for deprecations, so that the end users can auto complete the deprecations based on the static constants (of course, the generation logic is up to each implementor).

It does not conflict with the idea of parsing forVersion on the compiler side. In fact, I think we should do both.

@jathak
Copy link
Member

jathak commented May 21, 2024

Yeah, the more I think about this, it does make sense to support versions for fatal_deprecation in the embedded protocol in addition to the single-source-of-truth YAML file. I've filed #3873 to track a fast track proposal for this. This PR should be sufficient for the actual proposal, we'll need to wait at least two days for comment and I want to make sure #3872 (and its related PRs) are merged first, but otherwise this should all be good.

Regarding the behavior in pure JavaScript, that is indeed a bug. Only fatalDeprecations should support versions.

@ntkme
Copy link
Contributor Author

ntkme commented May 30, 2024

@jathak I've rebased all three PRs on top of #3872, would you mind take another look when you get a chance?

@jathak jathak merged commit c338c45 into sass:main May 30, 2024
9 checks passed
@ntkme ntkme deleted the deprecation-version branch May 30, 2024 23:35
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.

None yet

3 participants