-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Use plotly.min.js in both JS bundles #2103
Conversation
"clean": "rimraf dist/ && rimraf ../../python/plotly/plotlywidget/static'", | ||
"prepublish": "webpack", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this prepublish
thing is deprecated. It seems like NPM tries to run it when doing npm install
and fails unless you have webpack
installed globally for some reason.
@@ -22,10 +22,8 @@ | |||
"style/*.*" | |||
], | |||
"scripts": { | |||
"build": "npm run build:src", | |||
"build:src": "rimraf dist && tsc", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't a TypeScript module so this doesn't seem to do much
@@ -22,10 +22,8 @@ | |||
"style/*.*" | |||
], | |||
"scripts": { | |||
"build": "npm run build:src", | |||
"build:src": "rimraf dist && tsc", | |||
"build": "webpack", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build for this package is, in fact webpack, and running npm run build
does use the installed webpack in node_modules. So now we have to run npm install && npm run build
and there are no deprecation warnings :)
@@ -31,7 +31,7 @@ | |||
"typescript": "~3.1.1" | |||
}, | |||
"dependencies": { | |||
"plotly.js": "^1.51.2", | |||
"plotly.js": "^1.52.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intend to downgrade plotly.js here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s an upgrade actually ;)
One question above, but this looks good to me, assuming you've smoke tested the extensions in JupyterLab. Thanks @nicolaskruchten! |
I’ve smoke tested by uninstalling, building and reinstalling both extensions... is that about right? |
Closes #1873 the opposite way from #1891 so as not to bloat the classic nbextension size. If we wanted to do things the #1891 way we would need to add minification in
plotlywidget/webpack.config.js
so that the nbextension stays small. I'm indifferent but this way seemed easy :)This PR also inlines the
randstr
function the same way, and removes theprepublish: webpack
line that fails if webpack isn't globally installed, as well as the nonfunctionalbuild
target that callstsc
, replacing them with abuild: webpack
target that gets called explicitly in the CI step.The bundle size savings of this change are substantial on a percentage basis: both bundles together are now roughly the size of either bundle, as expected, instead of twice. Also PS I resurrected the jupyterlab-chart-editor and it uses the
min
bundle right now so the savings should be compounded if all three are installed.(closes #1891 if merged!)