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

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

Closed
bernhardreiter opened this issue Jul 4, 2019 · 10 comments

Comments

@bernhardreiter
Copy link
Contributor

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
Copy link
Member

yGuy commented Jul 4, 2019

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
Copy link
Contributor Author

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 bernhardreiter changed the title Using the master version in package.json does not work Using the git urls with specific branch or commit version in package.json does not work Jul 5, 2019
@bernhardreiter
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

yGuy commented Jul 5, 2019

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
Copy link
Contributor Author

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
Copy link

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 👍

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
Copy link
Contributor Author

My suggestions for a solution are:

  • Adopt the practice of jQuery to only commit dist files to release (or similiar) versions
    or
  • Add a hint to the documentation that people are likely to see (maybe in the readme and in the directory).

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
Copy link
Member

yGuy commented Jul 8, 2019

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
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants