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

Minified file added to web3 package #3131

Merged
merged 4 commits into from
Oct 16, 2019
Merged

Minified file added to web3 package #3131

merged 4 commits into from
Oct 16, 2019

Conversation

nivida
Copy link
Contributor

@nivida nivida commented Oct 15, 2019

The minified file which is located in the root folder of the 1.x branch will now also be published on npm. The minified file got defined as the entry file for browsers in packages/web3/package.json. I've also added this file to the .gitignore for not having it duplicated on GitHub. (We can probably later anyways remove the minified file from Git)

Fixes #1041

@nivida nivida added Enhancement Includes improvements or optimizations In Progress Currently being worked on 1.x 1.0 related issues labels Oct 15, 2019
@nivida nivida removed the In Progress Currently being worked on label Oct 15, 2019
@nivida nivida requested a review from cgewecke October 15, 2019 12:00
@coveralls
Copy link

coveralls commented Oct 15, 2019

Coverage Status

Coverage remained the same at 84.651% when pulling 36baba4 on issue/1041 into f471981 on 1.x.

Copy link
Collaborator

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

LGTM! Nice.

Copy link
Contributor

@joshstevens19 joshstevens19 left a comment

Choose a reason for hiding this comment

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

I would add some instructions in the README in how they would go about finding the file etc.. unless we already have it. Apart from that looks good to merge.

@nivida nivida merged commit 03ed119 into 1.x Oct 16, 2019
@nivida nivida deleted the issue/1041 branch October 17, 2019 16:05
@nivida nivida mentioned this pull request Oct 21, 2019
11 tasks
@alcuadrado
Copy link

Sorry I'm late, but what's the rationale for this PR? To have a browser entry in the package.json? Is it nay different than the main one except for minification?

@cgewecke
Copy link
Collaborator

@alcuadrado

In #1041 there's a subset of users expecting a dist with a min when they install like this:

npm install web3

At the moment it looks like people are git cloning web3 & running npm build themselves to produce this artifact.

@alcuadrado
Copy link

I suspect that does reports may come from a poor release process in the past. It sounds like some releases had no build artifact at all. This may not be a real issue now.

@cgewecke
Copy link
Collaborator

@alcuadrado Mmmm. 1.2.1 installs without a dist - I think this could be a backport.

@alcuadrado
Copy link

It doesn't, but why should it have a dist? It has a main in the package.json pointing to scr/index.js.

I think I'd just close the issue and revert this change until someone else reports it and give more info, if that ever happens.

Also, @nivida did some publishing work with rollup in the 2.x branch, which may conflict with this change (i.e. changing the expectations of what dist/ is). Even more so, if this project is to be migrated to TS (top priority after the backports?) the same may happen.

@alcuadrado
Copy link

I found that the main field was added here #1045 - Before that, there was no main, so it probably used to lead to issues when using it with web bundlers.

@cgewecke
Copy link
Collaborator

@alcuadrado Ok perhaps caution is reasonable here. As I read this change it only makes it possible to refer to a min via dist qualified path. We aren't testing webpack-ability per se but that might be a good idea.

One item in defense of the topicality of this PR is this (from the GH insights tab for Web3. Looking for the min is the 5th most common thing people do. 🤷‍♂
Screen Shot 2019-10-21 at 7 01 30 PM

@alcuadrado
Copy link

One item in defense of the topicality of this PR is this (from the GH insights tab for Web3. Looking for the min is the 5th most common thing people do. 🤷‍♂

This is really interesting! I've never used the GH traffic stats for this. Is it from the entire history? It'd be interesting to see if this keeps being true now, or if it mostly reflects how popular that issue was in the past.

@cgewecke
Copy link
Collaborator

@alcuadrado

Is it from the entire history?

Good question - it looks like it's configurable out to 1 month, via a drop-down option on the "insights/pulse" tab. Just ran that search and found more recent discussion about what & why people want this in #2623 as well....

@cgewecke
Copy link
Collaborator

@alcuadrado Sorry - I'm wrong about it being configurable lol. No idea what the time span is.

@nivida
Copy link
Contributor Author

nivida commented Oct 22, 2019

Sorry I'm late, but what's the rationale for this PR? To have a browser entry in the package.json? Is it nay different than the main one except for minification?

@alcuadrado The gulp build pipeline does create way smaller minified files then webpack is doing. This probably because it is excluding specific packages. Additionally, will CDN's use our minified version instead of bundling the minified files on their own. Another case is that some (also me) do like to run npm install web3 and to include the minified file in a hacky way into an HTML header for doing some quick tests.

@nivida
Copy link
Contributor Author

nivida commented Oct 22, 2019

Comparsion

With minified files:
Bildschirmfoto 2019-10-22 um 08 53 04

Without minified files:
Bildschirmfoto 2019-10-22 um 08 52 23

The production build with webpack is also way faster with the minified files provided.

Bundled src/index.js:

var Web3 = require('web3');
console.log(Web3);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Enhancement Includes improvements or optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants