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

Es5 target #2472

Merged

Conversation

@SylvainCorlay
Copy link
Member

commented Jul 1, 2019

Summary

For custom widget authors:

The end result of this PR is that in order for a custom widget to work in JupyterLab 1.0, it must require @jupyter-widgets/base version 2. To work in the classic notebook or JupyterLab 0.35, it should require @jupyter-widgets/base version 1. We suggest that a custom widget be updated to depend on @jupyter-widgets/base version ^1 || ^2 in order to work in both the classic notebook, JupyterLab 0.35, and JupyterLab 1.0.

@SylvainCorlay

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

@jasongrout

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Can we just switch the base package and maybe controls and output to es5? I don't think there's any reason to switch the jlab manager to es5.

@jasongrout

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

(you can switch just those packages to es5 by putting those two lines in their tsconfigs, which would override the base config they are inheriting from)

@SylvainCorlay

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

The individual tsconfig files don't seem to be actually taken into account.

Maybe it has to do with the removal of the --project option to tsc.

@SylvainCorlay

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

Seems to work. Pushing soon (when wifi improves on Eurostar).

@jasongrout

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

what about this (on top of 01c1536 from this PR)

diff --git a/packages/base/test/tsconfig.json b/packages/base/test/tsconfig.json
index 5fbcff44..186097c0 100644
--- a/packages/base/test/tsconfig.json
+++ b/packages/base/test/tsconfig.json
@@ -1,9 +1,11 @@
 {
   "extends": "../../../tsconfigbase",
   "compilerOptions": {
-    "types": ["mocha"],
+    "lib": ["dom", "es5", "es2015.promise", "es2015.iterable"],
     "outDir": "build",
-    "rootDir": "src"
+    "rootDir": "src",
+    "target": "es5",
+    "types": ["mocha"]
   },
   "include": ["src/*"]
 }
diff --git a/packages/base/tsconfig.json b/packages/base/tsconfig.json
index a1e4ba13..cf27bd24 100644
--- a/packages/base/tsconfig.json
+++ b/packages/base/tsconfig.json
@@ -1,8 +1,10 @@
 {
   "extends": "../../tsconfigbase",
   "compilerOptions": {
+    "lib": ["dom", "es5", "es2015.promise", "es2015.iterable"],
     "outDir": "lib",
-    "rootDir": "src"
+    "rootDir": "src",
+    "target": "es5",
   },
   "include": ["src/*"]
 }
diff --git a/packages/controls/test/tsconfig.json b/packages/controls/test/tsconfig.json
index 21efb391..5e891665 100644
--- a/packages/controls/test/tsconfig.json
+++ b/packages/controls/test/tsconfig.json
@@ -1,9 +1,11 @@
 {
   "extends": "../../../tsconfigbase",
   "compilerOptions": {
-    "types": ["mocha"],
+    "lib": ["dom", "es5", "es2015.promise", "es2015.iterable"],
     "outDir": "build",
-    "rootDir": "src"
+    "rootDir": "src",
+    "target": "es5",
+    "types": ["mocha"]
   },
   "include": ["src/**/*"]
 }
diff --git a/packages/controls/tsconfig.json b/packages/controls/tsconfig.json
index b566925b..31060f20 100644
--- a/packages/controls/tsconfig.json
+++ b/packages/controls/tsconfig.json
@@ -1,8 +1,10 @@
 {
   "extends": "../../tsconfigbase",
   "compilerOptions": {
+    "lib": ["dom", "es5", "es2015.promise", "es2015.iterable"],
     "outDir": "lib",
     "rootDir": "src",
+    "target": "es5",
     "types": ["mathjax", "node"]
   },
   "include": ["src/**/*"],
diff --git a/tsconfigbase.json b/tsconfigbase.json
index 1c79db37..6e815981 100644
--- a/tsconfigbase.json
+++ b/tsconfigbase.json
@@ -5,7 +5,6 @@
     "declaration": true,
     "esModuleInterop": true,
     "incremental": true,
-    "lib": ["dom", "es5", "es2015.promise", "es2015.iterable"],
     "jsx": "react",
     "module": "esnext",
     "moduleResolution": "node",
@@ -14,7 +13,7 @@
     "noUnusedLocals": true,
     "preserveWatchOutput": true,
     "resolveJsonModule": true,
-    "target": "es5",
+    "target": "es2017",
     "types": []
   }
 }
@SylvainCorlay

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

Actually, not, it does not seem that we can change the target by overriding the target in individual tsconfig.json. It is not taken into account at all.

@jasongrout

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Actually, not, it does not seem that we can change the target by overriding the target in individual tsconfig.json. It is not taken into account at all.

It seems to work for me. Did you completely clean things out and reinstall the npm packages?

@SylvainCorlay

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

@jasongrout

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

I'm fine with either the PR as it stands, or (preferable to me) bumping to es2017 for everything except base and controls, as in my patch above.

I still think this involves a major version bump in base, but I think most widget libraries would be able to just add the major version as an 'or' in their semver dependencies, i.e., bqplot probably could release a new patch version with:

"@jupyter-widgets/base": "^1.0.0 || ^2.0.0",

as its dependency (and perhaps update the controls dev dependency as well?)

@jasongrout

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

It builds but produces es2017.

It definitely produces es5 for me. Perhaps just deleting the incremental compilation caches is enough to redo the build?

@maartenbreddels

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Note that you need to rm tsconfig.tsbuildinfo very often, tsc keeps a hash of the sources, and will not re-output if you change the output target.

@jasongrout

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Perhaps just deleting the incremental compilation caches is enough to redo the build?

yarn run clean should delete these.

@SylvainCorlay

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

yarn run clean should delete these.

Still no luck. Trying by deleting things manually.

@vidartf

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

I still think this involves a major version bump in base

I'm confused about this. I thought the compile target bump was the only argument for bumping base?

@SylvainCorlay SylvainCorlay force-pushed the SylvainCorlay:back-to-the-future branch from 01c1536 to 95788e3 Jul 1, 2019

@SylvainCorlay

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

Still no luck. Trying by deleting things manually.

I got it to work by actually nuking my conda environment and re-installing everything. 😖

@SylvainCorlay

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

Yeah! Test are passing and third-party widgets work.

@SylvainCorlay SylvainCorlay merged commit 5d5cb3e into jupyter-widgets:master Jul 1, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@SylvainCorlay SylvainCorlay deleted the SylvainCorlay:back-to-the-future branch Jul 1, 2019

@jasongrout

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

I'm confused about this. I thought the compile target bump was the only argument for bumping base?

The services dependency went from:

    "@jupyterlab/services": "^1.0.1 || ^2.0.0 || ^3.0.0",

to

    "@jupyterlab/services": "^4.0.0",

Since we use and re-export typings from @jupyterlab/services, having a major version bump there means we need a major version bump.

Part of the problem is that the base package has too many things. We need the major version bump for the manager, but not for 3rd party widgets. Unfortunately, that means that 3rd party widgets need to bump (or at least add a dependency version) if the manager changes too much.

Alternatively, we could just declare that the base package is really on honoring semver for third party widgets.

@vidartf

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

To sum up a longer discussion on gitter:

  • The API of the base JS package changed enough to warrant a major bump, mainly because we are re-exporting types from @jupyterlab/services.
  • If we want to claim that the Python packages follow semver, then we would strictly need to do a major bump of those as well.
  • The typings that changed are only on lesser used parts of the API, so there is an argument that while we should be strict with the JS package semver, we are probably OK breaking semver for the Python package, and only do a minor release.
@vidartf

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Personally, my vote would be on doing major release of the Python package, but not try to rush in all other feature-requests / refactoring that is currently waiting for a major release. Rather, those would be punted to version 9. Most users do not actually have semver ranges for their Python dependencies, so a major bump shouldn't have to be a big thing. Only the very nervous puts ranging on python packages, and I think it would be nice of us to give them a fighting chance.

@jasongrout

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Link to even longer gitter discussion that I think is more nuanced than the summary above: https://gitter.im/jupyter-widgets/Lobby?at=5d1a11088dff05627ba70136

@jasongrout

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

I'm in favor of:

  1. ipywidgets 7.5, not 8.0 (this is a js thing, not a python thing)
  2. major version bump to base (so 2.0)
  3. probably minor bump to controls, with controls depending on base ^1 || ^2 - that is if there weren't other backwards incompatible changes besides the base upgrade
  4. tell third party widget authors that their widgets should work fine in classic and jlab 0.35, but to get their widgets to work in jlab 1.0, they'll need to issue a patch release where they bump their js dependency from base ^1 to base ^1 || ^2.
@jasongrout

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

I think moving forward we can:

  • break base into base-widget and base-manager
  • make a restricted interface for the manager exposed on a widget that lives in base-widget
  • version base-widget and base-manager separately
  • not sure where to put the jlab registry token- probably base-widget since that is the only thing we want people to depend on

Then base-manager depends on base-widget, and third-party libraries depend on base-widget, and we have two version numbers to communicate to two separate groups of people.

And @vidartf points out that we probably want to bump the compilation target to es2017 as well.

I think this would be an excellent change for 8.0 (it is a js-only thing, so we could probably do it before 8.0 if we weren't bumping the compilation target)

@jasongrout

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

We could probably even leave the base-widget package just called base, and separate out the manager. That would make @jupyter-widgets/base 3.0, and base-manager depend on base 3.0. That would ease upgrading (custom widget authors would just need to update to ^1 || ^2 || ^3)

@SylvainCorlay

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

That sounds like a good plan.

@jasongrout jasongrout added this to the 7.5 milestone Jul 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.