-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
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.
i think is a perfect start! |
|
i fixed it, i messed up with the publish order. |
Thanks! That was quick |
Unfortunately, this fix doesn't work. The problem is because of the way that npm handles versions: if you have two versions, 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 |
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 |
@asturur The problem is that if you specify this in your "dependencies": {
"fabric": "^3.3.2-browser"
}, ...then it will always install version The only way to use the 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 |
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:
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. |
Well, yes, the point is indeed to avoid needing to install But since you can't install the browser version with
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
There is no debate. It is always best practice to use You seem to be misunderstanding how package-lock works: the way that package-lock works is that you specify versions with Also, the
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 |
You customers develop applications with your library, hence my comment about package-lock. |
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 [email protected] is better to use. |
Yes, but that's irrelevant to whether amCharts should use The reason why it is best practice to use And the reason for package-lock is to make applications reproducible. Even if amCharts did not use So package locks are not an argument against 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. |
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 |
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 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 ). |
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. |
@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 |
@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:
|
As i said, to move from optionalDeps to peerDeps we need a major change. So bumping fabric to version 4. The package you want exists and is called |
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.
The script temporarily overrides package.json contents with
optionalDependencies: {}
andversion: '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 originalnpm publish
parameters:node publish --dry-run
.The file is eslinted with .eslintrc.json. Closes #5735.