builder and builder-victory component are runtime deps? #413

ljharb · 2016-10-27T23:16:39Z

These components seem to be related to building - which is a dev dep concern - and since builder-victory-component depends on chokidar/fsevents, it won't compile on our Linux machines.

Do these things need to be runtime dependencies, or can they be devDependencies? Typically any building/compilation should be done at publish time, not at install time (postinstall scripts are deprecated).

ljharb · 2016-10-27T23:27:22Z

(Note that this is blocking Airbnb from being able to use victory)

ryan-roemer · 2016-10-27T23:39:02Z

@ljharb -- Can you help us understand where the fsevents dep is coming from? It's not a top-level dep for anything for us. It's not builder + the archetype per se -- it's something else.

Yes, builder and the prod archetype builder-victory-component need to be prod deps to be able to support git-based installs. In the future, we're looking at tools like https://github.com/FormidableLabs/publishr to do a last minute, not-saved-in-git swap of builder and builder-victory-component to devDependencies since they aren't needed from npm (where things are already prebuilt).

Please help us track down what is actually breaking things for you and I suspect a simple upgrade will fix it...

ryan-roemer · 2016-10-27T23:41:09Z

For helpful reference here is the list of our actual top-level dependencies:

ryan-roemer · 2016-10-27T23:44:02Z

Also, we'll take a PR with publishr for this ticket #186 if anyone wants to take a stab.

Otherwise, @boygirl and I can triage it with everything else on our plate 😉

ljharb · 2016-10-27T23:49:26Z

@ryan-roemer for one, webpack, which is a MASSIVE runtime dependency to support the very rare (and community-discouraged) use case of git installs.

In general, including anything that touches the filesystem as a runtime dependency, anywhere in the tree, is going to cause us problems - which includes almost every building/compiling tool that exists.

The specific issue I'm seeing is in node_modules/victory/node_modules/builder-victory-component/node_modules/babel-cli/node_modules/chokidar/node_modules/fsevents - in other words, you have babel-cli as a runtime dependency.

ljharb · 2016-10-27T23:52:31Z

If you want to support git installs, I'd recommend compiling to things that are stored in git, but are npmignored.

ryan-roemer · 2016-10-27T23:54:23Z

@ljharb -- I understand our dependencies, and thank you for the requested information.

As I stated, we understand the issue and the tradeoffs the current stack makes to support git installs. And I've laid out a sensible path forward with publishr to support both npm and git installs, both as efficiently as possible.

You're welcome to do any of:

take a stab at the publishr conversion to do a last-minute switch of builder and builder-victory-component to devDependencies as part of a workflow in the archetype tasks; (Use publishr to move all build dependencies to devDependencies #186) OR
diagnose and suggest an update for babel-cli to unbreak you; OR
wait for us to get some free time to get to this on our OSS roadmap.

Thanks!

ryan-roemer · 2016-10-28T00:04:14Z

@ljharb -- And to clarify

In general, including anything that touches the filesystem as a runtime dependency, anywhere in the tree, is going to cause us problems - which includes almost every building/compiling tool that exists.

From an npm install victory, there is no complation whatsoever from webpack or babel or whatnot. The postinstall is a noop from npm.

boygirl · 2016-10-28T00:21:33Z

@ljharb I have a bit of time to take a stab at the publishr conversion. I'll keep you posted

ljharb · 2016-10-28T00:22:30Z

If that's indeed the case, then I'm not sure why our npm install is failing. It may in fact not be the blocker I think - but it's still a very unnecessarily large dependency tree, which is also a problem.

@boygirl thanks‼

(Another alternative i thought of, btw, would be to make victory-git that can be git-installed, and let victory itself only work when npm-installed)

boygirl · 2016-10-28T00:27:24Z

@ljharb git installs are super, super useful for our development workflow for victory-native (since npm link is so spotty with react native), so I'm pretty disinclined to drop support for that in the victory packages.

ljharb · 2016-10-28T01:12:09Z

With react-native tho, doesn't the packager transpile through node_modules, such that you wouldn't need to do it on postinstall?

ascott · 2016-10-28T01:44:16Z

thanks for all your work on this @ljharb @boygirl @ryan-roemer! really appreciated!

ljharb · 2016-10-28T05:42:40Z

OK, so it turns out the problem is that I'd generated the shrinkwrap file on my Mac, and fsevents is a runtime optionalDependency of chokidar, which is a direct runtime dep of builder-victory-component, that fails to compile on our Ubuntu, but since it's optional, it doesn't fail the install there. However, once it was in my Mac's shrinkwrap file, it killed the npm install on Ubuntu (because fsevents won't compile there).

In short, this is still a serious problem and adds a lot of weight to our dep tree, but I don't think this is an immediate blocker anymore.

@boygirl We really appreciate any time you can put into whatever is needed to clean up the dependencies vs devDependencies (ie, so that nothing build-related is a package.json dependency) throughout the victory dep graph - now or later, but it's OK if it isn't super rapid.

Thanks again for everyone's fast responses!

boygirl · 2016-10-28T06:09:28Z

@ljharb the changes ended up being pretty non-invasive. I need to do a bit of a sanity check in the morning, but I think we can get this out by the weekend or Monday at the latest.

ljharb · 2016-10-28T07:52:47Z

@boygirl awesome, thanks!

boygirl · 2016-10-28T19:33:56Z

closed by #415. Released as [email protected]

ryan-roemer · 2016-10-28T19:46:28Z

And here's the new package.json diff for the release update:

$ publish-diff -o [email protected] -n [email protected]

Index: package.json
===================================================================
--- package.json    [email protected]
+++ package.json    [email protected]
@@ -1,7 +1,7 @@
 {
   "name": "victory",
-  "version": "0.13.1",
+  "version": "0.13.2",
   "description": "Data viz for React",
   "keywords": [
     "data visualization",
     "React",
@@ -24,27 +24,41 @@
   "homepage": "https://github.com/formidablelabs/victory",
   "scripts": {
     "postinstall": "cd lib || builder run npm:postinstall || (echo 'POSTINSTALL FAILED: If using npm v2, please upgrade to npm v3. See bug https://github.com/FormidableLabs/builder/issues/35' && exit 1)",
     "preversion": "builder run npm:preversion",
+    "postversion": "builder run npm:postversion",
+    "postpublish": "builder run npm:postpublish",
     "start": "builder run hot",
     "test": "builder run check",
-    "version": "builder run npm:version && git add dist && git commit -m \"Commit 'dist/' for publishing\""
+    "version": "builder run npm:version"
   },
   "dependencies": {
-    "builder": "~2.9.1",
-    "builder-victory-component": "^3.0.0",
-    "victory-chart": "^13.0.0",
-    "victory-core": "^9.0.1",
-    "victory-pie": "^7.0.0"
+    "victory-chart": "^13.1.0",
+    "victory-core": "^9.1.0",
+    "victory-pie": "^7.1.0"
   },
   "devDependencies": {
-    "builder-victory-component-dev": "^3.0.0",
+    "builder-victory-component-dev": "^3.1.0",
     "chai": "^3.2.0",
     "lodash": "^4.12.0",
     "mocha": "^2.3.3",
     "react": "^15.1.0",
     "react-addons-test-utils": "^15.1.0",
     "react-dom": "^15.1.0",
     "sinon": "^1.17.2",
-    "sinon-chai": "^2.8.0"
-  }
-}
+    "sinon-chai": "^2.8.0",
+    "builder": "^3.1.0",
+    "builder-victory-component": "^3.1.0"
+  },
+  "publishr": {
+    "dependencies": [
+      "^builder"
+    ],
+    "files": {}
+  },
+  "_publishr": [
+    {
+      "created": false,
+      "path": "package.json"
+    }
+  ]
+}

ljharb · 2016-10-28T20:22:04Z

@boygirl thanks! That diff doesn't match https://github.com/FormidableLabs/victory/blob/master/package.json#L36 though - builder-victory-component is still listed in "dependencies". https://unpkg.com/[email protected]/package.json is correct though, so maybe you still have some uncommitted changes locally?

ryan-roemer · 2016-10-28T20:37:26Z

@ljharb -- that's the entire job of what publishr does: git dirty changes for npm publishing that are undone in real git repo.

That's how we get the best of all worlds – – the npm Version is deliberately different than the Git version in a maintainable manner.

That's also why I showed the diff with publish-diff going off of the real packages in the registry. Because it's meant to be different than in github

boygirl · 2016-10-28T20:38:17Z

@ljharb we built a little tool called publishr to move the dependencies on postversion and put them back on postpublish, so this is correct. npm installs of victory will not include infrastructure dependencies from builder and builder-victory-component, but git installs still will as they are required to build dist and lib

boygirl · 2016-10-28T20:38:41Z

ha, jinx

ryan-roemer · 2016-10-28T20:39:32Z

haha, totally

ljharb · 2016-10-28T20:51:49Z

aha, ok gotcha. Thanks for explaining, and for coming up with a solution!

williaster · 2016-10-29T00:14:54Z

thank you guys so much!!!

ryan-roemer added the Status: Needs More Info ✋ A question or report that needs more info to be addressable label Oct 27, 2016

boygirl closed this as completed Oct 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

builder and builder-victory component are runtime deps? #413

builder and builder-victory component are runtime deps? #413

ljharb commented Oct 27, 2016

ljharb commented Oct 27, 2016

ryan-roemer commented Oct 27, 2016

ryan-roemer commented Oct 27, 2016

ryan-roemer commented Oct 27, 2016

ljharb commented Oct 27, 2016

ljharb commented Oct 27, 2016

ryan-roemer commented Oct 27, 2016 •

edited

Loading

ryan-roemer commented Oct 28, 2016

boygirl commented Oct 28, 2016

ljharb commented Oct 28, 2016

boygirl commented Oct 28, 2016

ljharb commented Oct 28, 2016

ascott commented Oct 28, 2016

ljharb commented Oct 28, 2016

boygirl commented Oct 28, 2016 •

edited

Loading

ljharb commented Oct 28, 2016

boygirl commented Oct 28, 2016

ryan-roemer commented Oct 28, 2016

ljharb commented Oct 28, 2016

ryan-roemer commented Oct 28, 2016 •

edited

Loading

boygirl commented Oct 28, 2016

boygirl commented Oct 28, 2016

ryan-roemer commented Oct 28, 2016

ljharb commented Oct 28, 2016

williaster commented Oct 29, 2016

builder and builder-victory component are runtime deps? #413

builder and builder-victory component are runtime deps? #413

Comments

ljharb commented Oct 27, 2016

ljharb commented Oct 27, 2016

ryan-roemer commented Oct 27, 2016

ryan-roemer commented Oct 27, 2016

ryan-roemer commented Oct 27, 2016

ljharb commented Oct 27, 2016

ljharb commented Oct 27, 2016

ryan-roemer commented Oct 27, 2016 • edited Loading

ryan-roemer commented Oct 28, 2016

boygirl commented Oct 28, 2016

ljharb commented Oct 28, 2016

boygirl commented Oct 28, 2016

ljharb commented Oct 28, 2016

ascott commented Oct 28, 2016

ljharb commented Oct 28, 2016

boygirl commented Oct 28, 2016 • edited Loading

ljharb commented Oct 28, 2016

boygirl commented Oct 28, 2016

ryan-roemer commented Oct 28, 2016

ljharb commented Oct 28, 2016

ryan-roemer commented Oct 28, 2016 • edited Loading

boygirl commented Oct 28, 2016

boygirl commented Oct 28, 2016

ryan-roemer commented Oct 28, 2016

ljharb commented Oct 28, 2016

williaster commented Oct 29, 2016

ryan-roemer commented Oct 27, 2016 •

edited

Loading

boygirl commented Oct 28, 2016 •

edited

Loading

ryan-roemer commented Oct 28, 2016 •

edited

Loading