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

Possibly incorrect dagre-d3 version in package.json #1065

Closed
crusty opened this issue Nov 12, 2019 · 22 comments
Closed

Possibly incorrect dagre-d3 version in package.json #1065

crusty opened this issue Nov 12, 2019 · 22 comments
Assignees
Labels
Retained Nonperishable Status: Approved Is ready to be worked on Type: Bug / Error Something isn't working or is incorrect

Comments

@crusty
Copy link

crusty commented Nov 12, 2019

Hi,
I'm experiencing an issue while trying to install version ~8.4 via npm.

Let me expose it in an empty environment:
sudo docker run --rm -it node:12.3-alpine sh -c "npm i -g --loglevel verbose mermaid@~8.4"

This unfortunately results in:

npm info it worked if it ends with ok
npm verb cli [
npm verb cli   '/usr/local/bin/node',
npm verb cli   '/usr/local/bin/npm',
npm verb cli   'i',
npm verb cli   '-g',
npm verb cli   '--loglevel',
npm verb cli   'verbose',
npm verb cli   'mermaid@~8.4'
npm verb cli ]
npm info using npm@6.9.0
npm info using node@v12.3.1
npm verb npm-session 6d5644b94274e0d7
npm http fetch GET 200 https://registry.npmjs.org/mermaid 160ms
npm timing stage:loadCurrentTree Completed in 209ms
npm timing stage:loadIdealTree:cloneCurrentTree Completed in 1ms
npm timing stage:loadIdealTree:loadShrinkwrap Completed in 2ms
npm http fetch GET 200 https://registry.npmjs.org/crypto-random-string 181ms
npm http fetch GET 200 https://registry.npmjs.org/d3 182ms
npm http fetch GET 200 https://registry.npmjs.org/graphlib 178ms
npm http fetch GET 200 https://registry.npmjs.org/he 179ms
npm http fetch GET 200 https://registry.npmjs.org/minify 183ms
npm http fetch GET 200 https://registry.npmjs.org/lodash 193ms
npm http fetch GET 200 https://registry.npmjs.org/moment-mini 194ms
npm http fetch GET 200 https://registry.npmjs.org/prettier 120ms
npm http fetch GET 200 https://registry.npmjs.org/dagre 213ms
npm http fetch GET 200 https://registry.npmjs.org/scope-css 138ms
npm http fetch GET 200 https://registry.npmjs.org/@braintree%2fsanitize-url 1167ms
npm timing stage:rollbackFailedOptional Completed in 1ms
npm timing stage:runTopLevelLifecycles Completed in 1408ms
npm verb stack Error: spawn git ENOENT
npm verb stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:264:19)
npm verb stack     at onErrorNT (internal/child_process.js:456:16)
npm verb stack     at processTicksAndRejections (internal/process/task_queues.js:84:9)
npm verb cwd /
npm verb Linux 4.15.0-66-generic
npm verb argv "/usr/local/bin/node" "/usr/local/bin/npm" "i" "-g" "--loglevel" "verbose" "mermaid@~8.4"
npm verb node v12.3.1
npm verb npm  v6.9.0
npm ERR! path git
npm ERR! code ENOENT
npm ERR! errno ENOENT
npm ERR! syscall spawn git
npm ERR! enoent Error while executing:
npm ERR! enoent undefined ls-remote -h -t ssh://git@github.com/dagrejs/dagre-d3.git
npm ERR! enoent 
npm ERR! enoent 
npm ERR! enoent spawn git ENOENT
npm ERR! enoent This is related to npm not being able to find a file.
npm ERR! enoent 
npm verb exit [ 1, true ]
npm timing npm Completed in 1843ms

npm ERR! A complete log of this run can be found in:
npm ERR!     /root/.npm/_logs/2019-11-12T16_01_29_757Z-debug.log

This issue might be related with: ca5e60b, since the version specified here is not numerical.

@crusty crusty added Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect labels Nov 12, 2019
@vi-me-vi

This comment has been minimized.

@knsv
Copy link
Collaborator

knsv commented Nov 13, 2019

I am having issues reproducing this without docker).

First I thought this might be related to you using node 12 as we do not officially suppott that yet so I tries first with node 10 and then with node 12 on macos.

Here with node v10
image

Then I tried with node v12.
image

Could you try with node 10 and and with yarn and se if we can earn anything more?

@IOrlandoni IOrlandoni added Status: Pending Is not to be executed as it currently is and removed Status: Triage Needs to be verified, categorized, etc labels Nov 13, 2019
@klemmchr
Copy link
Member

klemmchr commented Nov 13, 2019

This is reproducible with node:12.3-alpine and node:12.3-slim but not in node:12.3. This basically happens because git is missing in alpine and slim but not in the full docker image. Installing git manually before installing mermaid works fine too.

In the package.json it is referenced as a git. I would assume this has been done because the latest release from dagre-d3 on npm is one year old. Is there a way we can fix that without big hassle?

@klemmchr klemmchr added Retained Nonperishable and removed Status: Pending Is not to be executed as it currently is labels Nov 13, 2019
@IOrlandoni IOrlandoni added the Status: Pending Is not to be executed as it currently is label Nov 14, 2019
@crusty
Copy link
Author

crusty commented Nov 18, 2019

I think it shouldn't be done in this way, especially that:

  1. The dependency version can be marked as "at least" (ie. >=version)
  2. I haven't noticed the practice of requiring git for any other node modules.

This would essentially make any slim and alpine docker images useless for anyone who uses mermaid for their project.

@klemmchr
Copy link
Member

klemmchr commented Nov 18, 2019

The dependency version can be marked as "at least" (ie. >=version)

That requires that the package is available on npm which isn't the case. Like I mentioned already the latest release of dagre-d3 is one year old.

I haven't noticed the practice of requiring git for any other node modules.

I agree that this is bad practice. Especially for docker users and as most builds these days are done in slim or alpine docker images this is a bad thing too. Also this is trappy for enterprise users sitting behind a restrictive firewall like in #1071.

However I can't verify exactly why there is a hard dependency on a GitHub repo. I will investigate that issue and see what we can do about this. If the npm version breaks stuff we might need to get the GitHub version into the project as a hard dependency.

As a workaround you can build the project in a full node image (or install git manually) and have it run in an alpine image.

@klemmchr klemmchr self-assigned this Nov 18, 2019
@klemmchr klemmchr removed the Status: Pending Is not to be executed as it currently is label Nov 18, 2019
@nothingismagick
Copy link
Contributor

I might be indirectly to blame, as I raised the issue in this discussion:
#990

@klemmchr
Copy link
Member

@nothingismagick Well, it passed the review so no blame here. However, I would be glad if you could take this issue and search a solution that doesn’t involve referencing GitHub packages.

@nothingismagick
Copy link
Contributor

It does seem a bit strange that the changes to dagre-d3 were merged, but no release since 2017!!!

If this doesn't get resolved I see two options: import it as a git submodule (which might actually have the same / similar / worse issue in docker) or flat-forking the repo and binding the source into mermaid itself (which is an option I don't like either...)

@klemmchr
Copy link
Member

klemmchr commented Nov 18, 2019

Yeah, that are really bad options. Either way we have the source with us which is a PITA. Plus, Submodules are a nightmare. The question is how fast we need to react to this issue. We now have at least 2 confirmed users that have a setup that they are affected. @knsv @dunning-kruger whats your opinion on that? I would say like 2 weeks, maybe contact the author via mail, he's a Microsoft employee so he might want to maintain his stuff at least with minimalistic efford.

Plus: do we have any alternatives?

@nothingismagick
Copy link
Contributor

nothingismagick commented Nov 18, 2019 via email

@klemmchr
Copy link
Member

Alternative would be to fork and publish master to npm as dagre-d3-mermaid. If that's wanted, I can do it tonight.

I thoght of that but I consider that as the worst solution because then we need to maintain another repo.

@IOrlandoni
Copy link
Member

The unofficial publication of dagre-3d would be my choice, under a name such as "dagre-3d-unofficial" or something like that, making it VERY clear that it is not an official release.

Maintaining it should not be an issue: all we do is point people to the official repo. If we need to, we can pull changes from upstream and re-release (but this should be rare, I guess).

I would keep the fork in the mermaid org just for "security" and future reference but, it's not to be maintained or developed on.

We should still try to contact the owner for a release, since that is of course the best option.

@klemmchr
Copy link
Member

Alright, so what's the time schedule then? @nothingismagick already commented on the pr, so wait a week, contact via mail, wait another week, create the fork?

@nothingismagick
Copy link
Contributor

nothingismagick commented Nov 18, 2019 via email

@damiandennis
Copy link

Hey Guys,

Any update on this? are we still waiting on the author of the dependency to get back?

Cheers

@nothingismagick
Copy link
Contributor

Yeah, its been a week, and my advice to host a fork at this org is probably the best way forward.

@knsv
Copy link
Collaborator

knsv commented Nov 27, 2019

Ok, I have forked the repo to mermaid-js.

@knsv
Copy link
Collaborator

knsv commented Nov 27, 2019

Just to summarize, we need an unofficial realase of the head of the fork to the npm repo, (not githubs repo)?

Then we update the dependencies in mermaid in release/8.4.3 branch wich is on its way out.

@klemmchr
Copy link
Member

Just to summarize, we need an unofficial realase of the head of the fork to the npm repo, (not githubs repo)?

Then we update the dependencies in mermaid in release/8.4.3 branch wich is on its way out.

Exactly, we need the latest master as a npm package (like dagre-d3-mermaid) so we can get rid of it. I assume you want to do the release, do you also want to update the dependency?

@knsv
Copy link
Collaborator

knsv commented Nov 27, 2019

Yes, I'll do both

@knsv
Copy link
Collaborator

knsv commented Nov 27, 2019

This is now pushed to the 8.4.3 release branch and available from the github package repo, https://github.com/mermaid-js/mermaid/packages/46638

@crusty
Copy link
Author

crusty commented Nov 29, 2019

What's the estimated date of 8.4.3 being released and pushed to npm?

@crusty crusty closed this as completed Dec 2, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Dec 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Retained Nonperishable Status: Approved Is ready to be worked on Type: Bug / Error Something isn't working or is incorrect
Projects
None yet
Development

No branches or pull requests

7 participants