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 model version separate from package version #448

Merged
merged 3 commits into from Apr 13, 2022

Conversation

ianhi
Copy link
Collaborator

@ianhi ianhi commented Mar 31, 2022

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

This was already too low - previously was 3.3.1. With update of
doubleclick we now need to require 3.4
@ianhi ianhi requested a review from martinRenou March 31, 2022 21:47
@github-actions
Copy link
Contributor

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

@ianhi
Copy link
Collaborator Author

ianhi commented Mar 31, 2022

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

Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Thanks a lot for doing this!

# These should not be changed unless we introduce changes to communication between
# frontend and backend.
__TOOLBAR_MODEL_VERSION__ = "^0.10.5"
__CANVAS_MODEL_VERSION__ = "^0.10.5"
Copy link
Member

Choose a reason for hiding this comment

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

It might be easier to track if we have one same version for all models?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah that's probably a good call. One isn't really functional without the other so no need to have them separate.

README.md Outdated
@@ -64,6 +64,7 @@ Versions lookup table:

| `ipympl` | `jupyter-matplotlib` | `JupyterLab` | `Matplotlib` |
|----------|----------------------|--------------|--------------|
| 0.8.9 | 0.10.x | 3 or 2 | 3.4.0>= |
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if we do version bumps separately, if you don't mind

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "jupyter-matplotlib",
"version": "0.10.5",
"version": "0.10.6",
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here for the version bump

@martinRenou
Copy link
Member

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
Copy link
Collaborator Author

ianhi commented Apr 1, 2022

  • removed version bumping stuff
  • Used same number for toolbar and canvas
  • model version now 1.0.0

@ianhi
Copy link
Collaborator Author

ianhi commented Apr 1, 2022

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'


@@ -32,6 +32,6 @@ function activateWidgetExtension(
registry.registerWidget({
name: MODULE_NAME,
version: MODULE_VERSION,
exports: () => import('./index'),
exports: (): any => import('./index'),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this gets rid of the typescript error, but is probably not the best way to do that.

@ianhi
Copy link
Collaborator Author

ianhi commented Apr 1, 2022

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

# how to speak. See counterpart in the src/version.ts file.
# These should not be changed unless we introduce changes to communication between
# frontend and backend.
__MODEL_VERSION__ = "^1.0.0"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in ipywidgets there is no ^ maybe we should remove also?

@wholtz
Copy link

wholtz commented Apr 1, 2022

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
Copy link
Member

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 :)

to allow compatibility between patch releases.
@@ -12,8 +12,8 @@ export class ToolbarModel extends DOMWidgetModel {
_view_name: 'ToolbarView',
_model_module: 'jupyter-matplotlib',
_view_module: 'jupyter-matplotlib',
_model_module_version: '^' + MODULE_VERSION,
_view_module_version: '^' + MODULE_VERSION,
_model_module_version: MODEL_VERSION,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure having the ^ does anything - and ipywidgets doesn't do this, so I removed it to be more similar to ipywidgets.

@ianhi
Copy link
Collaborator Author

ianhi commented Apr 5, 2022

I think this is ready now (supposing tests pass)

@ianhi ianhi changed the title A model version + bump version Add model version separate from package version Apr 12, 2022
@ianhi ianhi merged commit 8911332 into matplotlib:main Apr 13, 2022
@ianhi ianhi deleted the model-version branch April 13, 2022 17:36
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.

Incompatibilities between patch versions when installed in both server and kernel environment
3 participants