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

breaking: change biblio format #425

Merged
merged 4 commits into from
Mar 31, 2022
Merged

breaking: change biblio format #425

merged 4 commits into from
Mar 31, 2022

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Mar 27, 2022

This changes the imported/exported biblio format from being an object with a single key which is the location and whose value is the biblio entries to being an object with "location" and "entries" keys holding those values. This is nicer for tooling.

That is:

  • before: { "https://tc39.es/ecma262": [whatever] }
  • after: { "location": "https://tc39.es/ecma262", "entries": [whatever] }

This is a breaking change. Hopefully no one is depending on the new format yet. If you are, sorry about that; this will hopefully be a small change for you, and should be the last breaking change for the foreseeable future.

I considered taking this further, and changing from a list of objects with type keys to an object with one key for each valid type, but that's actually marginally harder to work with - if you want all the entries you have to coalesce the values for each key and modify them to include the type, which is a more annoying operation the .filter you have to use to go from the current format to having the entries of a particular type.

While I was at it, I also

  • added a warning for emu-biblio elements without an href
  • added a warning for emu-biblio elements inside of emu-import'd documents (which have never worked)

I could split that change out but I am lazy.

@bakkot bakkot requested a review from michaelficarra March 27, 2022 06:44
@bakkot bakkot force-pushed the better-exported-biblio branch from 4af66f9 to 819059c Compare March 27, 2022 06:46
@@ -9,7 +9,7 @@
<emu-clause id="sec-foo">
<h1><span class="secnum">1</span> Autolinking</h1>
<p>Type, type, <emu-xref aoid="Type"><a href="https://tc39.es/ecma262/#sec-ecmascript-data-types-and-values">Type</a></emu-xref>(), type()</p>
<p><emu-xref href="#sec-array-constructor"><a href="https://tc39.es/ecma262/#sec-array-constructor">%Array%</a></emu-xref> and <emu-xref href="#sec-properties-of-the-array-prototype-object"><a href="https://tc39.es/ecma262/#sec-properties-of-the-array-prototype-object">%ArrayPrototype%</a></emu-xref> from ES6 should link (but not %Arrayprototype%).</p>
<p><emu-xref href="#sec-array-constructor"><a href="https://tc39.es/ecma262/#sec-array-constructor">%Array%</a></emu-xref> and %ArrayPrototype% from ES6 should link (but not %Arrayprototype%).</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes to the baselines are because I updated the biblio file used for rendering baselines (to adopt the new format).

@bakkot
Copy link
Contributor Author

bakkot commented Mar 27, 2022

Hm, actually, Temporal is using a biblio in the old format with multiple keys. The new format doesn't accommodate multiple keys.

Probably the biblio-loading codes should check to see if it's given an array and treat that as an array of biblios.

Edit: done.

@bakkot bakkot mentioned this pull request Mar 27, 2022
@bakkot bakkot force-pushed the better-exported-biblio branch from 4675ba0 to 5099d4b Compare March 31, 2022 00:52
@bakkot bakkot merged commit 258197b into main Mar 31, 2022
@bakkot bakkot deleted the better-exported-biblio branch March 31, 2022 01:57
@rbuckton
Copy link
Contributor

rbuckton commented May 17, 2022

How was this nicer for tooling? If your biblio contained multiple entries, it could easily have been iterated over using Object.entries. Now you have to wrap it in a flatMap to handle a {...biblio...} vs [{ ...biblio... }].

Also, if you upgrade ecmarkup in an existing repo with a biblio, it now errors but gives no context as to why its an error. Instead it just crashes with biblio.entries is not iterable, which is a bit opaque. Would it have been feasible to test for the older format and issue a warning instead?

@bakkot
Copy link
Contributor Author

bakkot commented May 17, 2022

How was this nicer for tooling? If your biblio contained multiple entries, it could easily have been iterated over using Object.entries. Now you have to wrap it in a flatMap to handle a {...biblio...} vs [{ ...biblio... }].

I don't think a flatMap is any less nice than an Object.entries? And in the common case where you are dealing with exactly one biblio it's nicer not to have to key off of a URL or to do something like Object.values(o)[0]. Multiple people asked me for this change and I also thought it was an improvement personally.

Would it have been feasible to test for the older format and issue a warning instead?

Sure, pull requests welcome, or I'll get to it at some point. In general, though, I don't recommend upgrading major versions without reading the release notes, which document breaking changes. This particular change was called out in bold, even.

@FrankYFTang
Copy link

@sffc @gibson042 @ryzokuken Many TG2 proposals will need to reference to both ECMA262 and ECMA402. Could you clarify how could we do so in the future?
Also I found it is confusing that https://tc39.es/ecmarkup/#emu-biblio mention nothing about the new format.

@bakkot
Copy link
Contributor Author

bakkot commented Sep 21, 2022

You can export a biblio using --write-biblio some-file.json, and you can import a biblio using --load-biblio file-or-npm-package. If you want to load multiple biblios, you can repeat --load-biblio.

If you want the biblio from 402 to be easily referenced, it's probably best to automatically publish its biblio to npm, like 262 does. I can set that up if you'd like.

@FrankYFTang
Copy link

If you want the biblio from 402 to be easily referenced, it's probably best to automatically publish its biblio to npm, like 262 does. I can set that up if you'd like.

I think that will be nice. I will appreciate ECMA402 editors @leobalter @gibson042 @ryzokuken work with you about that.

Open issue tc39/ecma402#710 to track that.

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