search

yWorks / svg2pdf.js

A javascript-only SVG to PDF conversion utility that runs in the browser. Brought to you by yWorks - the diagramming experts
MIT License
654 stars 100 forks source link

Using the git urls with specific branch or commit version in package.json does not work #102

Closed bernhardreiter closed 5 years ago

bernhardreiter commented 5 years ago

yarn add https://github.com/yWorks/svg2pdf.js.git#master will checkout the latest version from git, but will not update the files which are in dist. Those are old, so while believing you have a never version, you haven't.

yGuy commented 5 years ago

What do you propose? We should not rebuild the files for every commit. The git repo will explode. We cannot remove these files without doing a major incompatible release (and I think we shouldn't).

Who does this anyway? That's what NPM is for. If you are fiddling with git versions in your package.json, you should be aware of this problem. Others do it just the same, don't they?

bernhardreiter commented 5 years ago

To me it is not sure what to do and howthe situation can be improved. I wrote the report to show that two people here ran into the problem. Now it is documented for discussions and others to find and hopefully avoid the problem.

From reading the npm documentation at https://docs.npmjs.com/files/package.json#git-urls-as-dependencies the expectation is raised that using a github url with branch or commit detail will lead to this version being used.

So far I cannot say if changing the install or build script in svg2pdf.js' package.json will lead to this expected result. I agree with you that rebuilding the files in dist for each commit maybe a bit much and I don't know yet how others do it.

bernhardreiter commented 5 years ago

One solution we are thinking of is to use our clone on this platform to actually rebuild the dist files for each commit when we need it. This probably can be automated somehow. However I'd prefer a better solution.

HackbrettXXX commented 5 years ago

I agree with @yGuy: it makes no sense to commit the dist files in every commit. And depending on the master branch is especially risky, since it may change any time and may introduce new bugs that are fixed later. The only stable versions are those marked with version tags. And on these commits the dist files are up to date.

If you need to depend on a specific commit then the best solution is to fork the repo and depend on it. You can switch back to an npm dependency once we released the changesets you require.

yGuy commented 5 years ago

Of course, if you depend on a specific commit, you can just checkout that commit and run the build manually in your deployment chain. Other than that: the fact that with each tagged release the dist is or should be up-to-date really makes this a very minor issue, IMHO.

bernhardreiter commented 5 years ago

Thanks for your comments, I agree that the issue is minor, once known. It can lead to a bad experience if not known (as we were debugging in the wrong direction).

Searching the web shows that there is no good practice for how to update dist yet. As a precaution someone could include the dist files only on release tag versions (see https://stackoverflow.com/questions/50974474/how-to-save-dist-build-files-on-github-release-but-not-in-master where jQuery is mentioned as a good example with this practice.)

See that the discussion in https://stackoverflow.com/questions/17509669/how-to-install-an-npm-package-from-github-directly shows that some people expect a package to installable from git and specify a script for doing so. However https://docs.npmjs.com/misc/scripts currently only recommends using preinstall for "compilation which must be done on the target architecture".

ThomasJunk commented 5 years ago

Hi,

I am the second developer (co-worker of Bernhard) who ran besides him into said problem. The main problem for me was an incorrect understanding how npm/yarn does the build process. The assumption, that it checks out and reads the sources and generates the according "dist" was a bit naive - to say the least. That caught me pants down.

@HackbrettXXX you are totally correct in stating that relying on master is a bit like playing with fire - it keeps you warm, but could also burn you. So at first, I pinned our version to a specific commit. That prevented said scenario of getting future unwanted stuff. But instead we ran into the at this time unexpected problems of the wonderful world of node packaging.

ATM I see this mostly as lessons learned in the npm-universe. For now, I forked the repo and made a build for us to work. So the immediate problem for me (us) is resolved.

Looking at your release history I see several releases this year which is from a "consumer-perspective" nice. Since @yGuy asked about possible proposals. I think, a proposal might be: If the release gap is "longer" ( I am aware of the subjective nature here) having something like rc-tags as in between releases with a proper dist would be nice to have. OTOH I am fully aware that this depends on your manpower, workload etc. and doing releases biyearly is totally fine. But I was asked for proposals (="wishes") ;)

And I am more than grateful, that you guys make this happen :thumbsup:

From my POV this issue could be closed - and as @bernhardreiter said is only of historical value in case someone besides us falls into the for him unexpected pit.

bernhardreiter commented 5 years ago

My suggestions for a solution are:

Additionally helpful would have been a version string in the minified result that indicates the development nature, if the file was build from inbetween commits.

yGuy commented 5 years ago

I vote for the "improve the documentation" option, but this really is a very minor issue. Most people would (and should) be using NPM packages using npm and there, this is a non-issue. In fact for the majority of people that new bit of documentation would be misleading because it would always be wrong/not applicable. We might consider not committing the dist files at all, in the future, anymore. So everyone who actually clones the repo will have to build the files themselves. And everyone using NPM properly gets the files, anyway. We are going to create a new release, not too far in the future, once our workload permits this, but it's just the same with hundreds of other repositories: If you depend on a specific version (or bugfix that hasn't been released), your best option is to (temporarily) clone the repo, fix the issue and depend on your repo. With history rewriting you can never depend on a git repo. You can only depend on npm versions.

bernhardreiter commented 5 years ago

@yGuy thanks for your comment and maintaining svg2pdf as Free Software!

My suggestion for a documentation hint:

If you want to use a development version from the repository, pay attention to the fact that the files in dist may reflect the last release version. So a simple package.json dependency link to the branch or revision will fail. See #102 for details.

To me it is more than a minor issue, because even if only a few people fall into this trap, the trap is several work-hours deep. And it would be a bad experience with svg2pdf nontheless it was caused by the way the npm/yarn world handles this.