Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[build] Don't require running npm install within mapbox-gl-js submodule #7938

Closed
wants to merge 2 commits into from

Conversation

jfirebaugh
Copy link
Contributor

No description provided.

Copy link
Contributor

@kkaefer kkaefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to fail with certain versions of npm. Locally, I'm using node 4.7.2 (the LTS version), which comes with npm 2.15.11. It installs the dependencies of the "@mapbox/mapbox-gl-test-suite": "file:mapbox-gl-js/test/integration" module into node_modules/@mapbox/mapbox-gl-test-suite/node_modules, which can't be found by the regular require call. Bitrise seems to use node 4.7.3 (which also comes with 2.15.11 when downloaded from nodejs.org), but somehow it's using npm 4.1.2 instead. That npm version doesn't nest node_modules folders anymore and just installs all dependencies in the node_modules folder.

Additionally, I'm concerned that using the file: syntax copies the test suite to the new installation location, meaning there are now two
instances of the test suite data, which might lead to confusion, and in particular makes modifying the tests hard.

@kkaefer
Copy link
Contributor

kkaefer commented Feb 6, 2017

Running make style-code fails with Error: Cannot find module 'sort-object', which is only installed by mapbox-gl-js

@kkaefer
Copy link
Contributor

kkaefer commented Feb 6, 2017

Also requires lodash.isequal, and jsonlint-lines-primitives.

@jfirebaugh
Copy link
Contributor Author

jfirebaugh commented Feb 6, 2017

What are the stack traces for those errors? As far as I can see, those dependencies are needed only if the style-spec root index.js is required, which this PR bypasses by requiring style-spec/reference/latest instead.

@lucaswoj lucaswoj mentioned this pull request Feb 6, 2017
@kkaefer
Copy link
Contributor

kkaefer commented Feb 7, 2017

For some reason, I can no longer replicate the issue with make style-code. However, I'm still getting errors when running the node render tests:

module.js:327
    throw err;
    ^

Error: Cannot find module 'd3-queue'
    at Function.Module._resolveFilename (module.js:325:15)
    at Function.Module._load (module.js:276:25)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at Object.<anonymous> (/Users/kkaefer/Code/gl/native/mapbox-gl-js/test/integration/lib/harness.js:7:15)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Module.require (module.js:353:17)
Program ended with exit code: 1

@jfirebaugh
Copy link
Contributor Author

Do you still get that if you update to npm 3 or later?

@kkaefer
Copy link
Contributor

kkaefer commented Feb 8, 2017

@jfirebaugh I'm not sure we should rely on users upgrading their npm from the version that shipped with the stock installation.

@jfirebaugh
Copy link
Contributor Author

OK, I'll try different approaches.

@jfirebaugh jfirebaugh closed this Feb 8, 2017
@jfirebaugh jfirebaugh deleted the update-gl-js branch February 8, 2017 19:18
@kkaefer
Copy link
Contributor

kkaefer commented Feb 9, 2017

Don't get me wrong, I think there's a lot of value in removing the npm install for mapbox-gl-js, if we can find a way to just install the parts that are required for the test suite. Alternatively, we could also up the minimum required node version to v6.

@jfirebaugh
Copy link
Contributor Author

94a3c27#diff-e43a15f1b424c5893699e4225835cc9d should do for now -- next steps are to see if we can remove npm install from the Makefile, and update node-cmake to remove that as an npm dependency.

@kkaefer
Copy link
Contributor

kkaefer commented Feb 9, 2017

there's a new version of node-cmake that explicitly doesn't use npm and instead advises you to copy the cmake file into your project. I've encountered 2 issues switching to the new version: cjntaylor/node-cmake#17 cjntaylor/node-cmake#18

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

Successfully merging this pull request may close these issues.

None yet

2 participants