Skip to content

Parse Deprecation.forVersion on compiler side#3868

Merged
jathak merged 1 commit intosass:mainfrom
ntkme:deprecation-version
May 30, 2024
Merged

Parse Deprecation.forVersion on compiler side#3868
jathak merged 1 commit intosass:mainfrom
ntkme:deprecation-version

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

3 participants