Dependency of Gulp 3.9.1 causes error in Node 10

[alexweissman]

There is a problem with natives@1.1.0 that is resolved by manually switching over to natives@1.1.3. See https://forums.userfrosting.com/t/upgrade-to-node-v10-generates-errors/281:

> gulp bower-install

gulp[5156]: ../src/node_contextify.cc:628:static void node::contextify::ContextifyScript::New(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `args[1]->IsString()' failed.
 1: node::Abort() [gulp]
 2: 0x557220a7229b [gulp]
 3: node::contextify::ContextifyScript::New(v8::FunctionCallbackInfo<v8::Value> const&) [gulp]
 4: v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo*) [gulp]
 5: 0x557220c7ce97 [gulp]
 6: 0x557220c7d68f [gulp]
 7: 0xc1f1ae0427d

npm ls shows the relevant dependency trace:

├─┬ gulp@3.9.1
│ └─┬ vinyl-fs@0.3.14
│   ├─┬ graceful-fs@3.0.11
│   │ └── natives@1.1.0

So, it seems like we are accumulating more and more problems related to Gulp 3.9.1.

[linkhousemedia]

Per @alexweissman, I have used a workaround and I've confirmed it worked.

Add "natives": "^1.1.3" to build/package.json in the devDependencies property.

[lcharette]

Hum... https://www.npmjs.com/package/graceful-fs/v/3.0.11

please upgrade to graceful-fs 4 for compatibility with current and future versions of Node.js

Anayway... According to graceful-fs dependencies, that should be fine. I'm not sure why it doesn't load 1.1.3 already. Is there another package depending on natives that could break because it requires less than 1.1.3 ?

[linkhousemedia]

Is there another package depending on natives that could break because it requires less than 1.1.3 ?

Possibly, but without this hack, I can't compile assets.

[Silic0nS0ldier]

I'd wager that the offending dependency is still hidden away but not used. Likely the addition of the dependency gives whatever is using it a newer version to use (that doesn't conflict with its semver range). Probably a dependency wanting ~1.0.0 (of similar) somewhere in the dependency tree.

Of course, its entirely plausible a bug in NPM is causing this to resolve to an older version. Wouldn't be the first time. Hard to say without looking further though.

[lcharette]

I tried to replicate any issue with assets on a fresh Homestead box and everything seems to work fine. Fresh UF master clone, tried with node 8 and 10.1.0

When running php bakery build-assets -c, everything works as it should out of the box for me. Might need to run php bakery build-assets -f -c if you already loaded some assets to force refresh the whole assets dir ? It would delete the lock file and force update everything, which might not be a desired result, but in UF case, shouldn't matter (?).

I also checked the dependencies tree in package-lock.json and npm ls and looks like graceful-fs is the only one requiring natives. So if it's still an issue with some of you, I guess we could do as @linkhousemedia said and add "natives": "^1.1.3" to our build/package.json, which seems to be a recommended fix.

[alexweissman]

I tried setting up a fresh install of UF with Node 7. Running npm ls | grep natives in build shows that natives@1.1.2 was installed as a part of the dependency graph.

When I upgraded to Node 10 and then ran build-assets again (with or without the -f flag), I saw the same error that @linkhousemedia was getting.

Deleting build/node_modules and build/package-lock.json, and then rerunning the build-assets command seemed to successfully install natives@1.1.3 and thus resolve the compatibility errors. However, as the issue that @lcharette found suggests, you might not want to do this in a production scenario.

It would appear that build-assets -f runs the uf-clean npm task, but this doesn't actually have anything to do with npm packages. uf-clean simply deletes the installed Bower packages from Sprinkles' assets/vendor directories, as well as compiled assets and the compiled asset bundle config file.

It's also worth noting that Node 10 is not yet officially recommended on the Node.js website - rather, as the LTS version, 8.11.1 is recommended for most users.

My takeaways from this issue are as follows:

• We could add natives@1.1.3 to the UF build/package.json file, but I don't know how that will play with older versions of UF. Since it's not a direct dependency of UF, I'm reluctant to add it as an official dependency just to fix this one, uncommon error scenario.
• We should consider factoring the build-assets Bakery command into multiple subcommands (perhaps build-assets:npm, build-assets:install, and build-assets:compile) with a flag that would let users clear the actual npm cache and/or delete the npm package lock file;
• This doesn't appear to be an issue in Node 8, the current LTS version. Furthermore, when I install UF on a fresh Vagrant box with Node 8, it seems to correctly install natives@1.1.3. So, this may only be an issue with Node <=7.
• The Gulp tasks are poorly named (uf-clean vs uf-assets-clean, etc). We should give them more intelligible names going forward.
• Overall, this demonstrates the fragility of the dependency graph. A "distribution repo", with all Composer, npm, and asset packages pre-installed, would help to avoid scaring away newbies who run into these sorts of issues as part of their first encounter with UF.

[Silic0nS0ldier]

Work that will hopefully land in 4.2 should push us up to gulp v4 which will resolve the issue. As for the 4.1 line, adding the newer version of natives seems to resolve this reliably enough so I'll see about getting it into a hotfix.

[galenzh]

delete yarn.lock and node_modules，use yarn command reinstall solve my problem.

[Silic0nS0ldier]

@zhangshuzhen Guessing you were using Yarn over npm. Effect is the same, it forced the package manager to completely rebuild the dependency tree. Does hint at something weird happening in the package managers though, because they should be able to resolve this problem themselves.

Changes mentioned before have landed in develop, so they are slatted for release in 4.2.

[Silic0nS0ldier]

4.2-beta.1 has been released which doesn't use the dependency in question at all.
The root cause of this (npm respecting the lockfile in an obtuse way) has caused issues for people migrating to the beta already, so a new issue has been created (#920) to get this sorted out once and for all.