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 simple upgrade/downgrade package for MyST AST #1802

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

agoose77
Copy link
Contributor

@agoose77 agoose77 commented Jan 23, 2025

This PR adds a simple package that sets an API for upgrading and downgrading an AST. Over time, we can use this to define imperative motions between AST schema versions.

For now, this PR implements a new "version 2" of the MyST schema which drops the use of number in favour of enumerator in footnotes.

Next Steps

  1. Hook this package into AST ingest routes in mystmd
  2. Separately update this package to pull in newer-than-mystmd releases

Copy link

changeset-bot bot commented Jan 23, 2025

🦋 Changeset detected

Latest commit: 35376ff

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
myst-compat Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@agoose77 agoose77 marked this pull request as ready for review January 23, 2025 17:41
Copy link
Member

@rowanc1 rowanc1 left a comment

Choose a reason for hiding this comment

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

I think the upgrade downgrade should be on the output of what is written to json in the site.

{
   version: 1,
   sha: "",
   mdast: {},
   citations: {order:} ... etc,
}

That way we can execute and move around.

Currently the upgrade/downgrade is on the mdast only (likely the bulk of the changes, but citations were a poor choice to do outside of the tree as an example...!).

import { downgrade } from './version_2_1.js';
import type { Parent } from 'mdast';

const SIMPLE_AST: Parent = {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it matters for now (as in we can get this in asap), however, I think this would be easier to manage with separate test dir with json files and then a global test runner.

Copy link
Collaborator

@fwkoch fwkoch left a comment

Choose a reason for hiding this comment

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

This is looking good to me - appreciate how it is simple and explicit.

import { upgrade as upgrade1To2 } from './version_1_2.js';

export function upgrade(from: string, to: string, ast: Parent): void {
if (from === '1' && to === '2') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This means we need to be careful and explicit about bumping version in the myst metadata.

An alternative could be having tests against the content to determine if the upgrade is required (e.g. "if footnoteReference numbering is defined") - that could allow more flexibility to introduce compatibility updates that missed a version bump on the myst side.

I think the explicit version numbering is good, just musing on alternatives.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a slightly different thought - do we need/want a minor_version field here? Where we can have a bit more intentionality around what constitutes a major change (outputs) vs something that is pretty minor and can be dealt with with fallbacks in the theme, etc.

Copy link
Contributor Author

@agoose77 agoose77 Jan 24, 2025

Choose a reason for hiding this comment

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

@fwkoch right now we need to do some work to ensure that we're talking about the same "version" (being the schema version), certainly.

I have flipped between thinking about using a minor version vs major version. These kind of changes are hard to categorise. Ultimately, my experience with Packaging leads me to feel that version numbers are not very good at communication compatibility when "compatibility" is not well-defined. And for MyST's AST schema, I think that's the case (it's used in so many different ways with differing assumptions).

The angle I've been working of late is to prefer handling compatibility explicitly by downgrading/upgrading, rather than implicitly encoding compatibility in the version number semantics.

Do either of you see a case where this might be a problem?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that for the schema, major/minor versioning are less useful. Any change to the schema could be breaking for some client I think. So then a minor bump would only mean something more like "this change isn't breaking for us"

It gets extra fuzzy with how we've used myst-spec-ext - because we have that package, the entire MyST development has happened with a single, unchanged version of myst-spec despite the AST extending and evolving. Even further, the site JSON Rowan mentioned in his review is defined by myst-cli only. So - does the upgrade/downgrade logic consider all those aspects and extended fields, but only is able to apply when the spec version changes?

@agoose77
Copy link
Contributor Author

I think the upgrade downgrade should be on the output of what is written to json in the site.

{
   version: 1,
   sha: "",
   mdast: {},
   citations: {order:} ... etc,
}

That way we can execute and move around.

Currently the upgrade/downgrade is on the mdast only (likely the bulk of the changes, but citations were a poor choice to do outside of the tree as an example...!).

I've been toying with the same idea too. Doing that would require that "version" here refers to both the AST version and the version of the page data. We probably can tie the two together.

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