matplotlib / ipympl

Matplotlib Jupyter Integration
https://matplotlib.org/ipympl/
BSD 3-Clause "New" or "Revised" License
1.57k stars 227 forks source link

Add model version separate from package version #448

Closed ianhi closed 2 years ago

ianhi commented 2 years ago

Hopefully fixes #416

  1. Adds a model version version that is not tied to the npm release version so that we can (hopefully) have compatibility across patch versions.

  2. Bumps the version for release of the double click fixes

github-actions[bot] commented 2 years ago

Binder :point_left: Launch a binder notebook on branch ianhi/ipympl/model-version

ianhi commented 2 years ago

An alternative approach that would make things simpler going forward would be to declare the current state as being model version 1.0.0 so we have the full range of semver to play with when making changes. The downside would be that we wouldn't get the back compatibility for this release. Not sure the best tradeoff there - curious for thoughts from @martinRenou and @wholtz

martinRenou commented 2 years ago

Concerning the version bump, what I did until now was to push release commits to master (see this for example), that keeps the release commit clean in the history and far from any merge commit.

ianhi commented 2 years ago
ianhi commented 2 years ago

any idea what's causing this new error?

src/plugin.ts:35:9 - error TS2322: Type '() => Promise<typeof import("/home/ian/Documents/oss/matplotlib/ipympl/src/index")>' is not assignable to type 'ExportData'.
  Type '() => Promise<typeof import("/home/ian/Documents/oss/matplotlib/ipympl/src/index")>' is not assignable to type '() => ExportMap'.
    Type 'Promise<typeof import("/home/ian/Documents/oss/matplotlib/ipympl/src/index")>' is not assignable to type 'ExportMap'.
      Index signature is missing in type 'Promise<typeof import("/home/ian/Documents/oss/matplotlib/ipympl/src/index")>'.

35         exports: () => import('./index'),
           ~~~~~~~

  node_modules/@jupyter-widgets/base/lib/registry.d.ts:35:5
    35     exports: ExportData;
           ~~~~~~~
    The expected type comes from property 'exports' which is declared here on type 'IWidgetRegistryData'
ianhi commented 2 years ago

Thank goodness for those galata tests. they catch that this actually does break things :/

wholtz commented 2 years ago

I don't have any strong opinion on declaring the current state as being model version 1.0.0. I suspect there isn't a way through this change without a bit of pain for users and I'd lean towards getting the painful parts over soon. Therefore I support the proposed way forward. Looking forward to this -- thanks @ianhi!

martinRenou commented 2 years ago

I suspect there isn't a way through this change without a bit of pain for users and I'd lean towards getting the painful parts over soon

I actually believe this could go smoothly for users. They will hopefully just see a version bump on ipympl and jupyter-matplotlib, as for any release. Then it will hopefully be easier for them to update one without the other from now on :)

ianhi commented 2 years ago

I think this is ready now (supposing tests pass)