Skip to content
This repository was archived by the owner on Feb 16, 2020. It is now read-only.

Adding minified files to dist packages - #1

Open
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files
Open

Adding minified files to dist packages#1
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files

Conversation

@cjwainwright

Copy link
Copy Markdown
Owner

The "dist" bundle packages (e.g. https://www.npmjs.com/package/plotly.js-dist) currently only include un-minified JavaScript. This PR is to include the minified version of the same "dist" file alongside this in the package.

It is common to import packages via npm but not have a minification step in your own build process. It is therefore helpful to have the minified version of the files included in the package.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@alexcjohnson@etpinard@antoinerg@archmoj Wondered if you’d take a look at this PR to include minified files in the dist packages. See also this comment here scijs/cwise#25 (comment)

@etpinard

Copy link
Copy Markdown

Thanks very much for making a PR @cjwainwright. Your patch looks good.

Now, here are the 👍 / 👎

👍

  • This PR would allow users to
    • npm i plotly.js-*dist
    • use node_modules/plotly.js-*dist/plotly-*.min.js
    • ... with no extra work

👎

  • This PR would increase the size of our plotly.js "dist" package by 30 to 50 percent (numbers taken from the plotly.js dist README) with arguably "duplicate" code.

Potential alternatives:

  • npm i plotly.js and use dist/plotly.min.js
    • but this downloads the flagged cwise package, some folks will find that bad even if they don't bundle their own code.
  • npm i plotly.js-*dist and minify node_modules/plotly.js-*dist/plotly-*.js
    • but not everyone will want to install a minifier
  • download the minified bundles from our CDN e.g. https://cdn.plot.ly/plotly-basic-latest.js
    • but some people want to use npm to install all their deps
  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

@archmoj

Copy link
Copy Markdown

Thanks @cjwainwright for the PR.
I was wondering if we could select only a few of those partial bundles, for example plotly.js-dist & plotly.js-gl3d-dist?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

Thank you for the feedback.

@etpinard Creating separate "dist min" packages sounds like the best approach, satisfying both the package size problem and allowing npm install of the min files. I can look at updating this to do that, unless you foresee any other issues with that?

@archmoj Can you explain why you would only include min files for some of the bundles? Would creating separate min packages be a good alternative from your perspective.

@etpinard

etpinard commented Jul 26, 2019

Copy link
Copy Markdown

The debate is still on between

  • adding min.js files to the current "dist" packages, and
  • publishing new "dist min" packages

We'll make a decision before the 1.50.0 release, which is scheduled for late August.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard@archmoj Was there any decision made about distributing minified files in dist packages yet? Thanks!

@etpinard

etpinard commented Sep 5, 2019

Copy link
Copy Markdown

@cjwainwright

We decided on this option:

  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

Would you be interesting in trying to make a PR?

@etpinard

Copy link
Copy Markdown

(and note 1.50.0 has been pushed back)

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard OK, great thanks. I had already made a PR on my fork for this (#2), if you're ok with that code I can look to make it as a PR back to the plotly repository.

@etpinard

Copy link
Copy Markdown

Thanks @cjwainwright - your patch looks good.


We should add some info about the new partial "dist min" bundles to

https://github.com/plotly/plotly.js/blob/master/dist/README.md

generated by

https://github.com/plotly/plotly.js/blob/master/tasks/stats.js

Maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L145

we could have:

Starting in `v1.50.0`, partial dist "min" packages are also published to npm.

and maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L210

we could have

### dist min npm package (starting in `v1.50.0`)
'',
'Install [`' + pkgName + '-dist`](https://www.npmjs.com/package/' + pkgName + '-dist) with',
'```',
'npm install ' + pkgName + '-dist',
'```',

What do you think?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard Sounds good, I can add those. I was thinking of making the wording a little more descriptive for the first part, maybe:

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Does that sound ok?

@etpinard

Copy link
Copy Markdown

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Yep, that's better than my attempt. Thanks!!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@cjwainwright@etpinard@archmoj@chris-wainwright
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Adding minified files to dist packages by cjwainwright · Pull Request #1 · cjwainwright/plotly.js · GitHub
Skip to content
This repository was archived by the owner on Feb 16, 2020. It is now read-only.

Adding minified files to dist packages - #1

Open
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files
Open

Adding minified files to dist packages#1
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files

Conversation

@cjwainwright

Copy link
Copy Markdown
Owner

The "dist" bundle packages (e.g. https://www.npmjs.com/package/plotly.js-dist) currently only include un-minified JavaScript. This PR is to include the minified version of the same "dist" file alongside this in the package.

It is common to import packages via npm but not have a minification step in your own build process. It is therefore helpful to have the minified version of the files included in the package.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@alexcjohnson@etpinard@antoinerg@archmoj Wondered if you’d take a look at this PR to include minified files in the dist packages. See also this comment here scijs/cwise#25 (comment)

@etpinard

Copy link
Copy Markdown

Thanks very much for making a PR @cjwainwright. Your patch looks good.

Now, here are the 👍 / 👎

👍

  • This PR would allow users to
    • npm i plotly.js-*dist
    • use node_modules/plotly.js-*dist/plotly-*.min.js
    • ... with no extra work

👎

  • This PR would increase the size of our plotly.js "dist" package by 30 to 50 percent (numbers taken from the plotly.js dist README) with arguably "duplicate" code.

Potential alternatives:

  • npm i plotly.js and use dist/plotly.min.js
    • but this downloads the flagged cwise package, some folks will find that bad even if they don't bundle their own code.
  • npm i plotly.js-*dist and minify node_modules/plotly.js-*dist/plotly-*.js
    • but not everyone will want to install a minifier
  • download the minified bundles from our CDN e.g. https://cdn.plot.ly/plotly-basic-latest.js
    • but some people want to use npm to install all their deps
  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

@archmoj

Copy link
Copy Markdown

Thanks @cjwainwright for the PR.
I was wondering if we could select only a few of those partial bundles, for example plotly.js-dist & plotly.js-gl3d-dist?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

Thank you for the feedback.

@etpinard Creating separate "dist min" packages sounds like the best approach, satisfying both the package size problem and allowing npm install of the min files. I can look at updating this to do that, unless you foresee any other issues with that?

@archmoj Can you explain why you would only include min files for some of the bundles? Would creating separate min packages be a good alternative from your perspective.

@etpinard

etpinard commented Jul 26, 2019

Copy link
Copy Markdown

The debate is still on between

  • adding min.js files to the current "dist" packages, and
  • publishing new "dist min" packages

We'll make a decision before the 1.50.0 release, which is scheduled for late August.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard@archmoj Was there any decision made about distributing minified files in dist packages yet? Thanks!

@etpinard

etpinard commented Sep 5, 2019

Copy link
Copy Markdown

@cjwainwright

We decided on this option:

  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

Would you be interesting in trying to make a PR?

@etpinard

Copy link
Copy Markdown

(and note 1.50.0 has been pushed back)

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard OK, great thanks. I had already made a PR on my fork for this (#2), if you're ok with that code I can look to make it as a PR back to the plotly repository.

@etpinard

Copy link
Copy Markdown

Thanks @cjwainwright - your patch looks good.


We should add some info about the new partial "dist min" bundles to

https://github.com/plotly/plotly.js/blob/master/dist/README.md

generated by

https://github.com/plotly/plotly.js/blob/master/tasks/stats.js

Maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L145

we could have:

Starting in `v1.50.0`, partial dist "min" packages are also published to npm.

and maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L210

we could have

### dist min npm package (starting in `v1.50.0`)
'',
'Install [`' + pkgName + '-dist`](https://www.npmjs.com/package/' + pkgName + '-dist) with',
'```',
'npm install ' + pkgName + '-dist',
'```',

What do you think?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard Sounds good, I can add those. I was thinking of making the wording a little more descriptive for the first part, maybe:

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Does that sound ok?

@etpinard

Copy link
Copy Markdown

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Yep, that's better than my attempt. Thanks!!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@cjwainwright@etpinard@archmoj@chris-wainwright
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Adding minified files to dist packages by cjwainwright · Pull Request #1 · cjwainwright/plotly.js · GitHub
Skip to content
This repository was archived by the owner on Feb 16, 2020. It is now read-only.

Adding minified files to dist packages - #1

Open
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files
Open

Adding minified files to dist packages#1
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files

Conversation

@cjwainwright

Copy link
Copy Markdown
Owner

The "dist" bundle packages (e.g. https://www.npmjs.com/package/plotly.js-dist) currently only include un-minified JavaScript. This PR is to include the minified version of the same "dist" file alongside this in the package.

It is common to import packages via npm but not have a minification step in your own build process. It is therefore helpful to have the minified version of the files included in the package.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@alexcjohnson@etpinard@antoinerg@archmoj Wondered if you’d take a look at this PR to include minified files in the dist packages. See also this comment here scijs/cwise#25 (comment)

@etpinard

Copy link
Copy Markdown

Thanks very much for making a PR @cjwainwright. Your patch looks good.

Now, here are the 👍 / 👎

👍

  • This PR would allow users to
    • npm i plotly.js-*dist
    • use node_modules/plotly.js-*dist/plotly-*.min.js
    • ... with no extra work

👎

  • This PR would increase the size of our plotly.js "dist" package by 30 to 50 percent (numbers taken from the plotly.js dist README) with arguably "duplicate" code.

Potential alternatives:

  • npm i plotly.js and use dist/plotly.min.js
    • but this downloads the flagged cwise package, some folks will find that bad even if they don't bundle their own code.
  • npm i plotly.js-*dist and minify node_modules/plotly.js-*dist/plotly-*.js
    • but not everyone will want to install a minifier
  • download the minified bundles from our CDN e.g. https://cdn.plot.ly/plotly-basic-latest.js
    • but some people want to use npm to install all their deps
  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

@archmoj

Copy link
Copy Markdown

Thanks @cjwainwright for the PR.
I was wondering if we could select only a few of those partial bundles, for example plotly.js-dist & plotly.js-gl3d-dist?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

Thank you for the feedback.

@etpinard Creating separate "dist min" packages sounds like the best approach, satisfying both the package size problem and allowing npm install of the min files. I can look at updating this to do that, unless you foresee any other issues with that?

@archmoj Can you explain why you would only include min files for some of the bundles? Would creating separate min packages be a good alternative from your perspective.

@etpinard

etpinard commented Jul 26, 2019

Copy link
Copy Markdown

The debate is still on between

  • adding min.js files to the current "dist" packages, and
  • publishing new "dist min" packages

We'll make a decision before the 1.50.0 release, which is scheduled for late August.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard@archmoj Was there any decision made about distributing minified files in dist packages yet? Thanks!

@etpinard

etpinard commented Sep 5, 2019

Copy link
Copy Markdown

@cjwainwright

We decided on this option:

  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

Would you be interesting in trying to make a PR?

@etpinard

Copy link
Copy Markdown

(and note 1.50.0 has been pushed back)

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard OK, great thanks. I had already made a PR on my fork for this (#2), if you're ok with that code I can look to make it as a PR back to the plotly repository.

@etpinard

Copy link
Copy Markdown

Thanks @cjwainwright - your patch looks good.


We should add some info about the new partial "dist min" bundles to

https://github.com/plotly/plotly.js/blob/master/dist/README.md

generated by

https://github.com/plotly/plotly.js/blob/master/tasks/stats.js

Maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L145

we could have:

Starting in `v1.50.0`, partial dist "min" packages are also published to npm.

and maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L210

we could have

### dist min npm package (starting in `v1.50.0`)
'',
'Install [`' + pkgName + '-dist`](https://www.npmjs.com/package/' + pkgName + '-dist) with',
'```',
'npm install ' + pkgName + '-dist',
'```',

What do you think?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard Sounds good, I can add those. I was thinking of making the wording a little more descriptive for the first part, maybe:

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Does that sound ok?

@etpinard

Copy link
Copy Markdown

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Yep, that's better than my attempt. Thanks!!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@cjwainwright@etpinard@archmoj@chris-wainwright
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Adding minified files to dist packages by cjwainwright · Pull Request #1 · cjwainwright/plotly.js · GitHub
Skip to content
This repository was archived by the owner on Feb 16, 2020. It is now read-only.

Adding minified files to dist packages - #1

Open
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files
Open

Adding minified files to dist packages#1
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files

Conversation

@cjwainwright

Copy link
Copy Markdown
Owner

The "dist" bundle packages (e.g. https://www.npmjs.com/package/plotly.js-dist) currently only include un-minified JavaScript. This PR is to include the minified version of the same "dist" file alongside this in the package.

It is common to import packages via npm but not have a minification step in your own build process. It is therefore helpful to have the minified version of the files included in the package.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@alexcjohnson@etpinard@antoinerg@archmoj Wondered if you’d take a look at this PR to include minified files in the dist packages. See also this comment here scijs/cwise#25 (comment)

@etpinard

Copy link
Copy Markdown

Thanks very much for making a PR @cjwainwright. Your patch looks good.

Now, here are the 👍 / 👎

👍

  • This PR would allow users to
    • npm i plotly.js-*dist
    • use node_modules/plotly.js-*dist/plotly-*.min.js
    • ... with no extra work

👎

  • This PR would increase the size of our plotly.js "dist" package by 30 to 50 percent (numbers taken from the plotly.js dist README) with arguably "duplicate" code.

Potential alternatives:

  • npm i plotly.js and use dist/plotly.min.js
    • but this downloads the flagged cwise package, some folks will find that bad even if they don't bundle their own code.
  • npm i plotly.js-*dist and minify node_modules/plotly.js-*dist/plotly-*.js
    • but not everyone will want to install a minifier
  • download the minified bundles from our CDN e.g. https://cdn.plot.ly/plotly-basic-latest.js
    • but some people want to use npm to install all their deps
  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

@archmoj

Copy link
Copy Markdown

Thanks @cjwainwright for the PR.
I was wondering if we could select only a few of those partial bundles, for example plotly.js-dist & plotly.js-gl3d-dist?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

Thank you for the feedback.

@etpinard Creating separate "dist min" packages sounds like the best approach, satisfying both the package size problem and allowing npm install of the min files. I can look at updating this to do that, unless you foresee any other issues with that?

@archmoj Can you explain why you would only include min files for some of the bundles? Would creating separate min packages be a good alternative from your perspective.

@etpinard

etpinard commented Jul 26, 2019

Copy link
Copy Markdown

The debate is still on between

  • adding min.js files to the current "dist" packages, and
  • publishing new "dist min" packages

We'll make a decision before the 1.50.0 release, which is scheduled for late August.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard@archmoj Was there any decision made about distributing minified files in dist packages yet? Thanks!

@etpinard

etpinard commented Sep 5, 2019

Copy link
Copy Markdown

@cjwainwright

We decided on this option:

  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

Would you be interesting in trying to make a PR?

@etpinard

Copy link
Copy Markdown

(and note 1.50.0 has been pushed back)

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard OK, great thanks. I had already made a PR on my fork for this (#2), if you're ok with that code I can look to make it as a PR back to the plotly repository.

@etpinard

Copy link
Copy Markdown

Thanks @cjwainwright - your patch looks good.


We should add some info about the new partial "dist min" bundles to

https://github.com/plotly/plotly.js/blob/master/dist/README.md

generated by

https://github.com/plotly/plotly.js/blob/master/tasks/stats.js

Maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L145

we could have:

Starting in `v1.50.0`, partial dist "min" packages are also published to npm.

and maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L210

we could have

### dist min npm package (starting in `v1.50.0`)
'',
'Install [`' + pkgName + '-dist`](https://www.npmjs.com/package/' + pkgName + '-dist) with',
'```',
'npm install ' + pkgName + '-dist',
'```',

What do you think?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard Sounds good, I can add those. I was thinking of making the wording a little more descriptive for the first part, maybe:

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Does that sound ok?

@etpinard

Copy link
Copy Markdown

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Yep, that's better than my attempt. Thanks!!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@cjwainwright@etpinard@archmoj@chris-wainwright
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Adding minified files to dist packages by cjwainwright · Pull Request #1 · cjwainwright/plotly.js · GitHub
Skip to content
This repository was archived by the owner on Feb 16, 2020. It is now read-only.

Adding minified files to dist packages - #1

Open
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files
Open

Adding minified files to dist packages#1
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files

Conversation

@cjwainwright

Copy link
Copy Markdown
Owner

The "dist" bundle packages (e.g. https://www.npmjs.com/package/plotly.js-dist) currently only include un-minified JavaScript. This PR is to include the minified version of the same "dist" file alongside this in the package.

It is common to import packages via npm but not have a minification step in your own build process. It is therefore helpful to have the minified version of the files included in the package.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@alexcjohnson@etpinard@antoinerg@archmoj Wondered if you’d take a look at this PR to include minified files in the dist packages. See also this comment here scijs/cwise#25 (comment)

@etpinard

Copy link
Copy Markdown

Thanks very much for making a PR @cjwainwright. Your patch looks good.

Now, here are the 👍 / 👎

👍

  • This PR would allow users to
    • npm i plotly.js-*dist
    • use node_modules/plotly.js-*dist/plotly-*.min.js
    • ... with no extra work

👎

  • This PR would increase the size of our plotly.js "dist" package by 30 to 50 percent (numbers taken from the plotly.js dist README) with arguably "duplicate" code.

Potential alternatives:

  • npm i plotly.js and use dist/plotly.min.js
    • but this downloads the flagged cwise package, some folks will find that bad even if they don't bundle their own code.
  • npm i plotly.js-*dist and minify node_modules/plotly.js-*dist/plotly-*.js
    • but not everyone will want to install a minifier
  • download the minified bundles from our CDN e.g. https://cdn.plot.ly/plotly-basic-latest.js
    • but some people want to use npm to install all their deps
  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

@archmoj

Copy link
Copy Markdown

Thanks @cjwainwright for the PR.
I was wondering if we could select only a few of those partial bundles, for example plotly.js-dist & plotly.js-gl3d-dist?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

Thank you for the feedback.

@etpinard Creating separate "dist min" packages sounds like the best approach, satisfying both the package size problem and allowing npm install of the min files. I can look at updating this to do that, unless you foresee any other issues with that?

@archmoj Can you explain why you would only include min files for some of the bundles? Would creating separate min packages be a good alternative from your perspective.

@etpinard

etpinard commented Jul 26, 2019

Copy link
Copy Markdown

The debate is still on between

  • adding min.js files to the current "dist" packages, and
  • publishing new "dist min" packages

We'll make a decision before the 1.50.0 release, which is scheduled for late August.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard@archmoj Was there any decision made about distributing minified files in dist packages yet? Thanks!

@etpinard

etpinard commented Sep 5, 2019

Copy link
Copy Markdown

@cjwainwright

We decided on this option:

  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

Would you be interesting in trying to make a PR?

@etpinard

Copy link
Copy Markdown

(and note 1.50.0 has been pushed back)

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard OK, great thanks. I had already made a PR on my fork for this (#2), if you're ok with that code I can look to make it as a PR back to the plotly repository.

@etpinard

Copy link
Copy Markdown

Thanks @cjwainwright - your patch looks good.


We should add some info about the new partial "dist min" bundles to

https://github.com/plotly/plotly.js/blob/master/dist/README.md

generated by

https://github.com/plotly/plotly.js/blob/master/tasks/stats.js

Maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L145

we could have:

Starting in `v1.50.0`, partial dist "min" packages are also published to npm.

and maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L210

we could have

### dist min npm package (starting in `v1.50.0`)
'',
'Install [`' + pkgName + '-dist`](https://www.npmjs.com/package/' + pkgName + '-dist) with',
'```',
'npm install ' + pkgName + '-dist',
'```',

What do you think?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard Sounds good, I can add those. I was thinking of making the wording a little more descriptive for the first part, maybe:

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Does that sound ok?

@etpinard

Copy link
Copy Markdown

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Yep, that's better than my attempt. Thanks!!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@cjwainwright@etpinard@archmoj@chris-wainwright
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Adding minified files to dist packages by cjwainwright · Pull Request #1 · cjwainwright/plotly.js · GitHub
Skip to content
This repository was archived by the owner on Feb 16, 2020. It is now read-only.

Adding minified files to dist packages - #1

Open
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files
Open

Adding minified files to dist packages#1
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files

Conversation

@cjwainwright

Copy link
Copy Markdown
Owner

The "dist" bundle packages (e.g. https://www.npmjs.com/package/plotly.js-dist) currently only include un-minified JavaScript. This PR is to include the minified version of the same "dist" file alongside this in the package.

It is common to import packages via npm but not have a minification step in your own build process. It is therefore helpful to have the minified version of the files included in the package.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@alexcjohnson@etpinard@antoinerg@archmoj Wondered if you’d take a look at this PR to include minified files in the dist packages. See also this comment here scijs/cwise#25 (comment)

@etpinard

Copy link
Copy Markdown

Thanks very much for making a PR @cjwainwright. Your patch looks good.

Now, here are the 👍 / 👎

👍

  • This PR would allow users to
    • npm i plotly.js-*dist
    • use node_modules/plotly.js-*dist/plotly-*.min.js
    • ... with no extra work

👎

  • This PR would increase the size of our plotly.js "dist" package by 30 to 50 percent (numbers taken from the plotly.js dist README) with arguably "duplicate" code.

Potential alternatives:

  • npm i plotly.js and use dist/plotly.min.js
    • but this downloads the flagged cwise package, some folks will find that bad even if they don't bundle their own code.
  • npm i plotly.js-*dist and minify node_modules/plotly.js-*dist/plotly-*.js
    • but not everyone will want to install a minifier
  • download the minified bundles from our CDN e.g. https://cdn.plot.ly/plotly-basic-latest.js
    • but some people want to use npm to install all their deps
  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

@archmoj

Copy link
Copy Markdown

Thanks @cjwainwright for the PR.
I was wondering if we could select only a few of those partial bundles, for example plotly.js-dist & plotly.js-gl3d-dist?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

Thank you for the feedback.

@etpinard Creating separate "dist min" packages sounds like the best approach, satisfying both the package size problem and allowing npm install of the min files. I can look at updating this to do that, unless you foresee any other issues with that?

@archmoj Can you explain why you would only include min files for some of the bundles? Would creating separate min packages be a good alternative from your perspective.

@etpinard

etpinard commented Jul 26, 2019

Copy link
Copy Markdown

The debate is still on between

  • adding min.js files to the current "dist" packages, and
  • publishing new "dist min" packages

We'll make a decision before the 1.50.0 release, which is scheduled for late August.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard@archmoj Was there any decision made about distributing minified files in dist packages yet? Thanks!

@etpinard

etpinard commented Sep 5, 2019

Copy link
Copy Markdown

@cjwainwright

We decided on this option:

  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

Would you be interesting in trying to make a PR?

@etpinard

Copy link
Copy Markdown

(and note 1.50.0 has been pushed back)

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard OK, great thanks. I had already made a PR on my fork for this (#2), if you're ok with that code I can look to make it as a PR back to the plotly repository.

@etpinard

Copy link
Copy Markdown

Thanks @cjwainwright - your patch looks good.


We should add some info about the new partial "dist min" bundles to

https://github.com/plotly/plotly.js/blob/master/dist/README.md

generated by

https://github.com/plotly/plotly.js/blob/master/tasks/stats.js

Maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L145

we could have:

Starting in `v1.50.0`, partial dist "min" packages are also published to npm.

and maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L210

we could have

### dist min npm package (starting in `v1.50.0`)
'',
'Install [`' + pkgName + '-dist`](https://www.npmjs.com/package/' + pkgName + '-dist) with',
'```',
'npm install ' + pkgName + '-dist',
'```',

What do you think?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard Sounds good, I can add those. I was thinking of making the wording a little more descriptive for the first part, maybe:

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Does that sound ok?

@etpinard

Copy link
Copy Markdown

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Yep, that's better than my attempt. Thanks!!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@cjwainwright@etpinard@archmoj@chris-wainwright
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Adding minified files to dist packages by cjwainwright · Pull Request #1 · cjwainwright/plotly.js · GitHub
Skip to content
This repository was archived by the owner on Feb 16, 2020. It is now read-only.

Adding minified files to dist packages - #1

Open
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files
Open

Adding minified files to dist packages#1
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files

Conversation

@cjwainwright

Copy link
Copy Markdown
Owner

The "dist" bundle packages (e.g. https://www.npmjs.com/package/plotly.js-dist) currently only include un-minified JavaScript. This PR is to include the minified version of the same "dist" file alongside this in the package.

It is common to import packages via npm but not have a minification step in your own build process. It is therefore helpful to have the minified version of the files included in the package.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@alexcjohnson@etpinard@antoinerg@archmoj Wondered if you’d take a look at this PR to include minified files in the dist packages. See also this comment here scijs/cwise#25 (comment)

@etpinard

Copy link
Copy Markdown

Thanks very much for making a PR @cjwainwright. Your patch looks good.

Now, here are the 👍 / 👎

👍

  • This PR would allow users to
    • npm i plotly.js-*dist
    • use node_modules/plotly.js-*dist/plotly-*.min.js
    • ... with no extra work

👎

  • This PR would increase the size of our plotly.js "dist" package by 30 to 50 percent (numbers taken from the plotly.js dist README) with arguably "duplicate" code.

Potential alternatives:

  • npm i plotly.js and use dist/plotly.min.js
    • but this downloads the flagged cwise package, some folks will find that bad even if they don't bundle their own code.
  • npm i plotly.js-*dist and minify node_modules/plotly.js-*dist/plotly-*.js
    • but not everyone will want to install a minifier
  • download the minified bundles from our CDN e.g. https://cdn.plot.ly/plotly-basic-latest.js
    • but some people want to use npm to install all their deps
  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

@archmoj

Copy link
Copy Markdown

Thanks @cjwainwright for the PR.
I was wondering if we could select only a few of those partial bundles, for example plotly.js-dist & plotly.js-gl3d-dist?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

Thank you for the feedback.

@etpinard Creating separate "dist min" packages sounds like the best approach, satisfying both the package size problem and allowing npm install of the min files. I can look at updating this to do that, unless you foresee any other issues with that?

@archmoj Can you explain why you would only include min files for some of the bundles? Would creating separate min packages be a good alternative from your perspective.

@etpinard

etpinard commented Jul 26, 2019

Copy link
Copy Markdown

The debate is still on between

  • adding min.js files to the current "dist" packages, and
  • publishing new "dist min" packages

We'll make a decision before the 1.50.0 release, which is scheduled for late August.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard@archmoj Was there any decision made about distributing minified files in dist packages yet? Thanks!

@etpinard

etpinard commented Sep 5, 2019

Copy link
Copy Markdown

@cjwainwright

We decided on this option:

  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

Would you be interesting in trying to make a PR?

@etpinard

Copy link
Copy Markdown

(and note 1.50.0 has been pushed back)

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard OK, great thanks. I had already made a PR on my fork for this (#2), if you're ok with that code I can look to make it as a PR back to the plotly repository.

@etpinard

Copy link
Copy Markdown

Thanks @cjwainwright - your patch looks good.


We should add some info about the new partial "dist min" bundles to

https://github.com/plotly/plotly.js/blob/master/dist/README.md

generated by

https://github.com/plotly/plotly.js/blob/master/tasks/stats.js

Maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L145

we could have:

Starting in `v1.50.0`, partial dist "min" packages are also published to npm.

and maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L210

we could have

### dist min npm package (starting in `v1.50.0`)
'',
'Install [`' + pkgName + '-dist`](https://www.npmjs.com/package/' + pkgName + '-dist) with',
'```',
'npm install ' + pkgName + '-dist',
'```',

What do you think?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard Sounds good, I can add those. I was thinking of making the wording a little more descriptive for the first part, maybe:

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Does that sound ok?

@etpinard

Copy link
Copy Markdown

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Yep, that's better than my attempt. Thanks!!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@cjwainwright@etpinard@archmoj@chris-wainwright
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Adding minified files to dist packages by cjwainwright · Pull Request #1 · cjwainwright/plotly.js · GitHub
Skip to content
This repository was archived by the owner on Feb 16, 2020. It is now read-only.

Adding minified files to dist packages - #1

Open
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files
Open

Adding minified files to dist packages#1
cjwainwright wants to merge 1 commit into
masterfrom
distribute-minified-files

Conversation

@cjwainwright

Copy link
Copy Markdown
Owner

The "dist" bundle packages (e.g. https://www.npmjs.com/package/plotly.js-dist) currently only include un-minified JavaScript. This PR is to include the minified version of the same "dist" file alongside this in the package.

It is common to import packages via npm but not have a minification step in your own build process. It is therefore helpful to have the minified version of the files included in the package.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@alexcjohnson@etpinard@antoinerg@archmoj Wondered if you’d take a look at this PR to include minified files in the dist packages. See also this comment here scijs/cwise#25 (comment)

@etpinard

Copy link
Copy Markdown

Thanks very much for making a PR @cjwainwright. Your patch looks good.

Now, here are the 👍 / 👎

👍

  • This PR would allow users to
    • npm i plotly.js-*dist
    • use node_modules/plotly.js-*dist/plotly-*.min.js
    • ... with no extra work

👎

  • This PR would increase the size of our plotly.js "dist" package by 30 to 50 percent (numbers taken from the plotly.js dist README) with arguably "duplicate" code.

Potential alternatives:

  • npm i plotly.js and use dist/plotly.min.js
    • but this downloads the flagged cwise package, some folks will find that bad even if they don't bundle their own code.
  • npm i plotly.js-*dist and minify node_modules/plotly.js-*dist/plotly-*.js
    • but not everyone will want to install a minifier
  • download the minified bundles from our CDN e.g. https://cdn.plot.ly/plotly-basic-latest.js
    • but some people want to use npm to install all their deps
  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

@archmoj

Copy link
Copy Markdown

Thanks @cjwainwright for the PR.
I was wondering if we could select only a few of those partial bundles, for example plotly.js-dist & plotly.js-gl3d-dist?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

Thank you for the feedback.

@etpinard Creating separate "dist min" packages sounds like the best approach, satisfying both the package size problem and allowing npm install of the min files. I can look at updating this to do that, unless you foresee any other issues with that?

@archmoj Can you explain why you would only include min files for some of the bundles? Would creating separate min packages be a good alternative from your perspective.

@etpinard

etpinard commented Jul 26, 2019

Copy link
Copy Markdown

The debate is still on between

  • adding min.js files to the current "dist" packages, and
  • publishing new "dist min" packages

We'll make a decision before the 1.50.0 release, which is scheduled for late August.

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard@archmoj Was there any decision made about distributing minified files in dist packages yet? Thanks!

@etpinard

etpinard commented Sep 5, 2019

Copy link
Copy Markdown

@cjwainwright

We decided on this option:

  • publish "dist min" package to the npm registry, that is all our "dist" packages would have a "dist min" equivalent so that users could then e.g. npm i plotly.js-basic-dist-min

Would you be interesting in trying to make a PR?

@etpinard

Copy link
Copy Markdown

(and note 1.50.0 has been pushed back)

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard OK, great thanks. I had already made a PR on my fork for this (#2), if you're ok with that code I can look to make it as a PR back to the plotly repository.

@etpinard

Copy link
Copy Markdown

Thanks @cjwainwright - your patch looks good.


We should add some info about the new partial "dist min" bundles to

https://github.com/plotly/plotly.js/blob/master/dist/README.md

generated by

https://github.com/plotly/plotly.js/blob/master/tasks/stats.js

Maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L145

we could have:

Starting in `v1.50.0`, partial dist "min" packages are also published to npm.

and maybe below line

https://github.com/plotly/plotly.js/blob/240334eb51f9abfa0237aadcae90f1d47b10a92a/tasks/stats.js#L210

we could have

### dist min npm package (starting in `v1.50.0`)
'',
'Install [`' + pkgName + '-dist`](https://www.npmjs.com/package/' + pkgName + '-dist) with',
'```',
'npm install ' + pkgName + '-dist',
'```',

What do you think?

@cjwainwright

Copy link
Copy Markdown
OwnerAuthor

@etpinard Sounds good, I can add those. I was thinking of making the wording a little more descriptive for the first part, maybe:

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Does that sound ok?

@etpinard

Copy link
Copy Markdown

Starting in v1.50.0, the minified version of each partial bundle is also published to npm in a separate "dist min" package.

Yep, that's better than my attempt. Thanks!!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@cjwainwright@etpinard@archmoj@chris-wainwright