[go: up one dir, main page]

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

Deprecate x_google_ignoreList in favor of ignoreList, remove Known Extensions #19

Merged
merged 3 commits into from
Oct 12, 2023

Conversation

EricSL
Copy link
Contributor
@EricSL EricSL commented Oct 5, 2023

Making a proposal to deprecate x_google_ignoreList in favor of a new field named ignoreList. I am also open to making the name thirdParty if people would prefer that, as it would name it according to what the information means rather than what a consumer might do with it.

If this change is agreed on and there is buy-in from someone at Mozilla, I can make the implementation change for Chromium.

@kamilogorek
Copy link
Member

Not sure about the "2024" part exactly, but otherwise looks good!

@EricSL
Copy link
Contributor Author
EricSL commented Oct 6, 2023

I wasn't sure about that part either. The full migration plan would look like this:

  1. Agree that we're switching to this name.
  2. Consumers (I think that is only Chromium + Firefox) currently supporting x_google_ignoreList switch to supporting both, with ignoreList taking precedence over x_google_ignoreList. This is pretty trivial to implement.
  3. New versions of browsers roll out and displace older browsers. I think we can be at this point by early 2024 if we get on this now, but not necessarily by January 1.
  4. Tools switch from emitting source maps with x_google_ignoreList to emitting with ignoreList.
  5. It takes much longer for new versions of tools to be integrated into every project.
  6. Some day browsers might look at usage metrics and decide x_google_ignoreList can be fully retired.

But I didn't want this feature to take up too much of the spec space, so rather than spell out a migration plan in detail I gave a vague timeline that I think is realistic.

@jkup
Copy link
Collaborator
jkup commented Oct 7, 2023

Thank you so much for submitting this! My first thought was to remove the x_google_ignoreList section entirely instead of calling it deprecated. Then maybe in the new ignoreList section, we could just mention that some browsers also support the x_google_ignoreList field but that ignoreList will always take priority if found.

What do you think about that?

@kamilogorek
Copy link
Member

This is how it was done for X-SourceMap, so it might be a good idea to move the deprecation note to the main section itself.
I wonder if at this rate (assuming ignoreLine will ever get into spec), by the end of the day, we will even need "extensions" section at all.

@EricSL
Copy link
Contributor Author
EricSL commented Oct 9, 2023

I could delete the Known Extensions section. I'm not aware of anyone using x_google_lineCount.

@kamilogorek
Copy link
Member

I'm all up for removing it entirely tbh. As you mentioned, x_google_lineCount has literally 5 results in the Google itself, and the only mention about it in the discussion is from 11 years ago at https://bugzilla.mozilla.org/show_bug.cgi?id=765993

@jkup
Copy link
Collaborator
jkup commented Oct 9, 2023

Sorry, just checking on the linecount I hadn't noticed it before!

Copy link
Member
@kamilogorek kamilogorek left a comment

Choose a reason for hiding this comment

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

The change looks good to me. I'd either split it into two commits, or update the PR title accordingly, as it does more than just updating ignoreList now.

@EricSL EricSL changed the title Deprecate x_google_ignoreList in favor of official field Deprecate x_google_ignoreList in favor of ignoreList, remove Known Extensions Oct 10, 2023
@bmeurer
Copy link
bmeurer commented Oct 12, 2023

@jkup anything else needed here?

@jkup
Copy link
Collaborator
jkup commented Oct 12, 2023

Thank you for doing this!

@jkup jkup merged commit 83ed289 into tc39:main Oct 12, 2023
2 checks passed
github-actions bot added a commit that referenced this pull request Oct 12, 2023
SHA: 83ed289
Reason: push, by jkup

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@nicolo-ribaudo
Copy link
Member

Hi @EricSL :) Quick question: do you work for a company that's part of TC39? If not, could you sign TC39's IPR form (see https://github.com/tc39/ecma262/blob/main/CONTRIBUTING.md#required-legal-agreements for more info)?

Unfortunately there is a bug in our CI scripts and they didn't catch it until the PR has been merged.

@EricSL
Copy link
Contributor Author
EricSL commented Oct 20, 2023

I work for Google and I see we are on this list: https://www.ecma-international.org/tc39-royalty-free-technical-committee-members/ so I believe there's no problem?

@nicolo-ribaudo
Copy link
Member

Yes then it's ok :) We can add an exception to the IPR check script so that it does not fail on your contribution (or we can ask to Google's delegates if it's possible to onboard you as a TC39 delegate, if you prefer).

@EricSL
Copy link
Contributor Author
EricSL commented Oct 23, 2023

I'd prefer the exception.

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

5 participants