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

Create a script for publishing #5786

Merged
merged 1 commit into from
Jul 2, 2019
Merged

Create a script for publishing #5786

merged 1 commit into from
Jul 2, 2019

Conversation

finom
Copy link
Contributor

@finom finom commented Jul 1, 2019

The script temporarily overrides package.json contents with optionalDependencies: {} and version: 'FABRIC_VERSION-browser', publishes it, restores the original package.json and publishes the main version.
It can be run with node publish and it also supports original npm publish parameters: node publish --dry-run.
The file is eslinted with .eslintrc.json. Closes #5735.

The script temporarily overrides package.json contents with `optionalDependencies: {}` and `version: 'FABRIC_VERSION-browser'`, publishes it, restores the original package.json and publishes the main version. 
It can be run with `node publish` and it also supports original `npm publish` parameters: `node publish --dry-run`. 
The file is eslinted with .eslintrc.json. Closes #5735.
@asturur
Copy link
Member

asturur commented Jul 2, 2019

i think is a perfect start!

@asturur asturur merged commit dfb0a6d into fabricjs:master Jul 2, 2019
@finom finom deleted the patch-2 branch July 2, 2019 10:30
@asturur asturur mentioned this pull request Jul 14, 2019
@Climax777
Copy link

yarn upgrade-interactive --latest now wants to upgrade my 3.2.0 to the 3.3.0-browser version after this.

@asturur
Copy link
Member

asturur commented Jul 15, 2019

i fixed it, i messed up with the publish order.
Now you will get 3.3.2 that is exactly 3.3.0 after a couple of publish try

@Climax777
Copy link

i fixed it, i messed up with the publish order.
Now you will get 3.3.2 that is exactly 3.3.0 after a couple of publish try

Thanks! That was quick

@Pauan
Copy link

Pauan commented Jul 22, 2019

Unfortunately, this fix doesn't work. The problem is because of the way that npm handles versions: if you have two versions, 1.0.0 and 1.0.0-foo, then 1.0.0 will have priority.

That's because the suffixes are intended for pre-release versions (alpha, beta, etc.) This is the intentional design of how semver works with npm.

So the only real solution is to publish a new fabric-browser npm package.

@asturur
Copy link
Member

asturur commented Jul 22, 2019

being able to specify version and add -browser after it, is still better than not having it at all.

You would not update fabric blindlessly without reading the changelog anyway, if you are using for a production app, is probably one of the main pieces of it.

When a new version is out, you will update your package.json

@Pauan
Copy link

Pauan commented Jul 22, 2019

@asturur The problem is that if you specify this in your package.json...

"dependencies": {
  "fabric": "^3.3.2-browser"
},

...then it will always install version 3.3.2, never 3.3.2-browser.

The only way to use the -browser version is to specify it exactly (without ^), but then you miss out on bug fixes and it can cause multiple versions of fabric to be installed at the same time (which is bad for multiple reasons).

That isn't acceptable for us (or, I imagine, for most users), and it also goes against npm best practices (which is to always specify ^ unless there's a very good reason not to).

@asturur
Copy link
Member

asturur commented Jul 22, 2019

So the whole point of having a browser specific version is just to help people that are not able to install node dependencies that can't for various reason not even try to do it.

As I said, being able to install a specific version, is still better than not having the possibility at all.

Going to your points:

  • it can cause multiple versions of fabric to be installed at the same time possible but unlikely to happen.
  • and it also goes against npm best practices (which is to always specify ^ unless there's a very good reason not to) I think this is a debated enough best practice. So debated that things like package-lock exist from npm itself.

Now, i'm sorry this is not acceptable for you, but as i stated, i have no time or will to mantain another package, if you think a dependency free version is going to help people more than this, please do it.

@Pauan
Copy link

Pauan commented Jul 22, 2019

So the whole point of having a browser specific version is just to help people that are not able to install node dependencies that can't for various reason not even try to do it.

Well, yes, the point is indeed to avoid needing to install canvas. As an example of the issues that canvas causes, here is a report just today from one of our users.

But since you can't install the browser version with ^, that makes it useless for libraries (like amCharts).

possible but unlikely to happen.

No, it's actually very likely. If two different npm libraries both use fabric, it's likely that they will use different versions. That wouldn't be the case if they used ^.

So debated that things like package-lock exist from npm itself.

There is no debate. It is always best practice to use ^ for versions in libraries.

You seem to be misunderstanding how package-lock works: the way that package-lock works is that you specify versions with ^ and then it locks the specific versions down in package-lock.json. You still should specify versions with ^.

Also, the package-lock.json is for applications, but we're talking about specifying the version in libraries.

i have no time or will to mantain another package

That is your choice, but I don't see the problem. This PR already added in a script which creates a browser-only version.

So the script just needs to be modified so that when you publish to fabric it will also automatically publish to fabric-browser. So there's 0 extra effort for you.

@asturur
Copy link
Member

asturur commented Jul 22, 2019

You customers develop applications with your library, hence my comment about package-lock.

@finom
Copy link
Contributor Author

finom commented Jul 22, 2019

Guys, before I made this PR, I've created this package https://www.npmjs.com/package/fabric-pure-browser which is published once a day from from fabric.js repository if a new version appears, so you can use ^ and stuff. I'm not going to remove it since it's powered by free tooling (Travis CI) and it has some downloads. But still, using fabric@x.y.z-browser is better to use.

@Pauan
Copy link

Pauan commented Jul 22, 2019

You customers develop applications with your library, hence my comment about package-lock.

Yes, but that's irrelevant to whether amCharts should use ^ or not: amCharts (like all npm libraries) should use ^, and then the final application will use package-lock to lock down all of the versions for all of the libraries (including dependencies of dependencies, etc.).

The reason why it is best practice to use ^ is because that prevents package duplication, which is a very common occurrence without ^. That's the whole reason why ^ was invented in the first place. And package duplication is especially bad for browsers, since it dramatically increases the file size.

And the reason for package-lock is to make applications reproducible. Even if amCharts did not use ^, the application would still need to use package locks, because dependencies of dependencies might use ^ (for example, fabric itself uses ^ for the versions of canvas and jsdom).

So package locks are not an argument against ^, in fact the whole reason package locks were invented is to make ^ better, thus encouraging more usage of ^.

If you don't believe me, we can discuss this with the npm team, and see what they have to say about version best practices. I do know some people who formerly worked at npm and Node.

@Climax777
Copy link

Climax777 commented Jul 23, 2019

fwiw I second @Pauan's suggestion to modify the script to publish to fabric-browser. It achieves the same result: a separate package for browser-only, while keeping true to the "way it should be used" like @Pauan is describing.

Furthermore, end-users will have the same experience: an alternate install option for browser-only

@asturur
Copy link
Member

asturur commented Jul 23, 2019

I want to refresh the main concept that the dependency we are complaining about do not make the installation fail.

The main concern here is people seeing logs of node-canvas failing the install and calling amCharts support for information instead of reading the log and understanding there is nothing to worry about.
I cannot make myself to justify a second package for that only reason. I already feel bad enough to duplicate the space taken on NPM archives with copied versions.

I would ask NPM people why there is no easy way to silence logs ( unless you have the .npmrc file in the root folder of the project to say so, but not in a dependency folder ) and if they think that not clean logs are worth duplication of packages.

Having a fabric-browser package with a readme that says that it works in node too, isn't that a bit misleading? There is no a clear win here, no one is stuck, fabric install correctly, there is a browser version in the main package and there is a side package with the browser only version ( if we could clarify the README that would be nice @finom, optional of course ).

@tvdstaaij
Copy link

This approach looks rather silly to me, because:

This PR introduces nonstandard versioning and breaks how npm is supposed to work, so the only thing it achieves is confusing users.

If fixing the root cause of the problem is impossible or too difficult publishing a separate package would be perfectly reasonable. Could be done with a helper script similar to the one in this PR so that it doesn't introduce much maintenance effort.

@finom
Copy link
Contributor Author

finom commented Sep 9, 2019

@asturur @tvdstaaij I propose to use npm tags for the browser-only version, see https://docs.npmjs.com/cli/publish. This would allow to keep latest tag clean and to close this discussion.

@Pauan
Copy link

Pauan commented Sep 9, 2019

@finom A tag would only allow users to install the latest version, not older versions. Thus, it completely breaks semver.

There are only two solutions to this problem:

  1. Use peerDependencies (which was my original proposed solution)

  2. Use a separate fabric-browser package

@asturur
Copy link
Member

asturur commented Sep 19, 2019

As i said, to move from optionalDeps to peerDeps we need a major change. So bumping fabric to version 4.
The fabric-browser package is free no one is using it, you can still do it as @finom was doing before.

The package you want exists and is called https://www.npmjs.com/package/fabric-pure-browser

thiagocunha pushed a commit to thiagocunha/fabric.js that referenced this pull request Nov 18, 2019
The script temporarily overrides package.json contents with `optionalDependencies: {}` and `version: 'FABRIC_VERSION-browser'`, publishes it, restores the original package.json and publishes the main version. 
It can be run with `node publish` and it also supports original `npm publish` parameters: `node publish --dry-run`. 
The file is eslinted with .eslintrc.json. Closes fabricjs#5735.
This pull request was closed.
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.

Get rid of node-canvas dependency
5 participants