linkedin / eyeglass

NPM Modules for Sass
741 stars 60 forks source link

Some imports still fail unexpectedly #70

Closed alanhogan closed 8 years ago

alanhogan commented 8 years ago

We've got a bower-managed package named zd-css-settings.weights containing a SCSS file we want to import.

The line is: @import '../../../bower_components/zd-css-settings.weights/settings.weights';

and it blows up thusly:

module.js:340
    throw err;
    ^
Error: Cannot find module 'zd-css-settings.weights/package.json'
    at Function.Module._resolveFilename (module.js:338:15)
    at resolve (/harmony/code/zendesk_console/node_modules/broccoli-eyeglass/node_modules/eyeglass/lib/util/resolve.js:19:17)
    at /harmony/code/zendesk_console/node_modules/broccoli-eyeglass/node_modules/eyeglass/lib/util/discover.js:151:17
    at Array.forEach (native)
    at discover (/harmony/code/zendesk_console/node_modules/broccoli-eyeglass/node_modules/eyeglass/lib/util/discover.js:150:25)
    at Object.getModules [as all] (/harmony/code/zendesk_console/node_modules/broccoli-eyeglass/node_modules/eyeglass/lib/util/discover.js:198:19)
    at Object.ImportUtilities.getModuleByName (/harmony/code/zendesk_console/node_modules/broccoli-eyeglass/node_modules/eyeglass/lib/import_utils.js:38:29)
    at Object.<anonymous> (/harmony/code/zendesk_console/node_modules/broccoli-eyeglass/node_modules/eyeglass/lib/module_importer.js:66:30)
    at options.importer (/harmony/code/zendesk_console/node_modules/node-sass/lib/index.js:321:31)
alanhogan commented 8 years ago

Having a hard time reproducing in the test suite though.

eoneill commented 8 years ago

Does zd-css-settings.weights have an eyeglass-exports.js?

I'd like to understand what's going on here more, but you might be able to work around the issue by adding ../../../bower_components to your includePaths (might want to use an absolute path).

Then here you'd @import "zd-css-settings.weights/settings.weights";

eoneill commented 8 years ago

Additionally, is zd-css-settings.weights in your package.json?

Looking through the logic, it should only be trying to resolve zd-css-settings.weights/package.json if zd-css-settings.weights was declared in your dependencies or devDependencies.

alanhogan commented 8 years ago

Thanks for your responses, @eoneill —

Does zd-css-settings.weights have an eyeglass-exports.js?

No, it only has a _foo.scss file, a package.json, a bower.json, and a README.md

I'd like to understand what's going on here more, but you might be able to work around the issue by adding ../../../bower_components to your includePaths (might want to use an absolute path). Then here you’d@import "zd-css-settings.weights/settings.weights";

I tried this — but frustratingly at present the file in question 'needs' to also be able to be compiled by Compass and Compass barfs when the ../../../bower_components part of this path is removed…

Additionally, is zd-css-settings.weights in your package.json?

Great question. No. It’s only managed by bower.

Looking through the logic, it should only be trying to resolve zd-css-settings.weights/package.json if zd-css-settings.weights was declared in your dependencies or devDependencies.

It sure doesn’t seem to be.

I’m struggling trying to make sense of the eyeglass logic here.

eoneill commented 8 years ago

If you remove the file with the above import, does the error go away?

alanhogan commented 8 years ago

@eoneill Does commenting out the import serve the same purpose? (A lot of different files import the same file, which imports a second file, which then makes the above import.)

eoneill commented 8 years ago

Yeah, that should give me the same info, does commenting out the import remove the error?

alanhogan commented 8 years ago

Okay, well, commenting out the @import line in question does eliminate the error. (Of course then I get errors for undefined variables.)

Is that helpful? :/

Okay, after that, I spent some time and was able to use includePaths in Eyeglass and the equivalent in Compass such that I could drop all the ../../bower_components/ from all my @import lines.

And wouldn’t you know it?

Getting the same exact error now, Cannot find module 'zd-css-settings.weights/package.json

Kind of at a loss here. I’m going to flail around a little more and see if anything interesting happens

eoneill commented 8 years ago

@chriseppstein @Jakobo any thoughts?

The stack trace is pointing to lib/util/discover.js:151, which should only ever resolve dependencies (from topPackage).

I can easily reproduce this error by adding an entry to package.json but not npm install it (e.g. "dependencies": { "not-a-real-npm-package": "*" }).

alanhogan commented 8 years ago

Really interesting. Even if the bogus package is a totally different name to whatever is being imported?

Alan

On Oct 15, 2015, at 6:27 PM, Eugene ONeill notifications@github.com wrote:

@chriseppstein @Jakobo any thoughts?

The stack trace is pointing to lib/util/discover.js:151, which should only ever resolve dependencies (from topPackage).

I can easily reproduce this error by adding an entry to package.json but not npm install it (e.g. "dependencies": { "not-a-real-npm-package": "*" }).

— Reply to this email directly or view it on GitHub.

eoneill commented 8 years ago

I still can't figure out how to reproduce this issue :)

Can you try patching you local eyeglass with this patch.

cd ./node_modules/broccoli-eyeglass/node_modules/eyeglass/
patch -p0 -i /path/to/eyeglass.patch

This will add some logging so we can try to see what's happening in your case.

alanhogan commented 8 years ago

Certainly, and thanks for your continued help with this issue.

Here is a gist with a lot of the output. (We're running multiple eyeglass builds in parallel, for a number of files. If the output is muddled — i think it‘s probably fine — but if you’d rather I think I can run a build for just one file at a time.)

https://gist.github.com/alanhogan/3ccc7562a12a0cb7016f

alanhogan commented 8 years ago

Oh man. I see the issue. zd-css-base.fonts has a package.json which is stating a dependency of zd-css-settings.weights. Which totally exists in a location that eyeglass can look in! But it’s not, say, nested within zd-css-base.fonts’s node_modules folder (which does not exist).

And as a reminder, these two files were installed to our project using bower, not npm. (I might — might! — be able to change this. And in the coming months those packages are due to be merged, anyway.)

alanhogan commented 8 years ago

Wanted to mention here — changing the package.json in question to list the other bower modules as peerDependencies instead fixes this issue.

In light of the causes of the situation here — do we think there is a potential to change the eyeglass discovery behavior at all, or is this all by design and copacetic with npm standards?

eoneill commented 8 years ago

@alanhogan #72 should address this. Can you try pointing your package.json to my fork and confirm this fixes the issue.

alanhogan commented 8 years ago

Hey @eoneill, I apologize for not getting back sooner. I ended up moving the bower_components in question over to npm to side-step this issue. (It should have also been possible to suppress package.json from the bower component, or remove the dependencies since it didn’t quite make sense to have nested deps in this context, but I wasn’t in control of the packages in question)

I can still test this though. I just need to make some time to revert the above changes and reproduce the issue again.

chriseppstein commented 8 years ago

Fixed in 0.7.0