Fix npm 2.x build failures for angular-material-source dependency #364

eriktrom · 2016-05-04T20:42:01Z

Angular-material-source does not have a main node module, its only
built to ./dist and published via bower. The first attempt to make
this work with npm 3.x broke (some users) npm 2.x env's.
This aims to allow building a project that includes this addon for
npme 2.x and 3.x for all versions of node.

fixes #349

- Angular-material-source does not have a main node module, its only built to ./dist and published via bower. The first attempt to make this work with npm 3.x broke (some users) npm 2.x env's. - This aims to allow building a project that includes this addon for npme 2.x and 3.x for all versions of node. fixes miguelcobain#349

eriktrom · 2016-05-04T20:46:32Z

Note the difference is the use of this.nodeModulesPath instead of this.project.nodeModulesPath

this.nodeModulesPath is defined in both addon.js and project.js

Addon: https://github.com/ember-cli/ember-cli/blob/feddfd099156b8aa13d21ee2ee903fa0c86e2408/lib/models/addon.js#L102

Project: https://github.com/ember-cli/ember-cli/blob/feddfd099156b8aa13d21ee2ee903fa0c86e2408/lib/models/project.js#L108

(crosses fingers)

@cah-danmonroe @thijsvdanker - do either of you mind giving this a whirl, thanks a bunch

DanChadwick · 2016-05-04T21:10:17Z

I checkout this commit and deleted angular-material-source from ember-paper/node_modules and my-project/node-modules. In ember-paper, I npm linked and it reinstalled angular-material-source. In my project, I npm installed and it did the same. It then built correctly. npm v2.14.7. I can't say for sure whether this worked before, but it certainly worked now. Windows, for what it's worth.

eriktrom · 2016-05-04T21:15:49Z

@DanChadwick - your awesome - once @cah-danmonroe or @thijsvdanker can verify it works for them, i think we can merge.

I never had an issue w/ 2.x and node 0.12 (and neither does travis). :shrugs:

cah-danmonroe · 2016-05-04T22:42:02Z

This commit didn't work for me either.
node v4.2.3, npm 2.14.7
on a mac

> /Users/daniel.monroe/projects/training/ember/eptest/node_modules/ember-paper/node_modules   master ✘ ✹ ✭
 ll                                                                                                                                                             
total 0
drwxr-xr-x   9 daniel.monroe  CARDINALHEALTH\Domain Users  306 May  4 18:36 .
drwxr-xr-x  18 daniel.monroe  CARDINALHEALTH\Domain Users  612 May  4 18:36 ..
drwxr-xr-x  24 daniel.monroe  CARDINALHEALTH\Domain Users  816 May  4 18:36 angular-material-source
drwxr-xr-x   7 daniel.monroe  CARDINALHEALTH\Domain Users  238 May  4 18:36 broccoli-autoprefixer
drwxr-xr-x   9 daniel.monroe  CARDINALHEALTH\Domain Users  306 May  4 18:36 broccoli-filter
drwxr-xr-x  14 daniel.monroe  CARDINALHEALTH\Domain Users  476 May  4 18:36 broccoli-funnel
drwxr-xr-x  18 daniel.monroe  CARDINALHEALTH\Domain Users  612 May  4 18:36 broccoli-merge-trees
drwxr-xr-x  12 daniel.monroe  CARDINALHEALTH\Domain Users  408 May  4 18:36 ember-css-transitions
drwxr-xr-x  14 daniel.monroe  CARDINALHEALTH\Domain Users  476 May  4 18:36 ember-wormhole

Here's my package.json:

> {
  "name": "eptest",
  "version": "0.0.0",
  "description": "Small description for eptest goes here",
  "private": true,
  "directories": {
    "doc": "doc",
    "test": "tests"
  },
  "scripts": {
    "build": "ember build",
    "start": "ember server",
    "test": "ember test"
  },
  "repository": "",
  "engines": {
    "node": ">= 0.10.0"
  },
  "author": "",
  "license": "MIT",
  "devDependencies": {
    "broccoli-asset-rev": "^2.4.2",
    "ember-ajax": "0.7.1",
    "ember-cli": "2.4.3",
    "ember-cli-app-version": "^1.0.0",
    "ember-cli-babel": "^5.1.6",
    "ember-cli-dependency-checker": "^1.2.0",
    "ember-cli-htmlbars": "^1.0.3",
    "ember-cli-htmlbars-inline-precompile": "^0.3.1",
    "ember-cli-inject-live-reload": "^1.4.0",
    "ember-cli-qunit": "^1.4.0",
    "ember-cli-release": "0.2.8",
    "ember-cli-sass": "5.3.1",
    "ember-cli-sri": "^2.1.0",
    "ember-cli-uglify": "^1.2.0",
    "ember-data": "^2.4.2",
    "ember-export-application-global": "^1.0.5",
    "ember-load-initializers": "^0.5.1",
    "ember-paper": "git+https://github.com/miguelcobain/ember-paper.git#6f95be81ea20eca54d1c6ee70eb30ef86372f3e5",
    "ember-resolver": "^2.0.3",
    "loader.js": "^4.0.1"
  }
}

cah-danmonroe · 2016-05-04T23:03:59Z

ember-paper> git pull origin pull/364/head
npm link

eptest> rm -rf node_modules
eptest> npm link ember-paper
eptest> npm install
eptest> ember build
Built project successfully. Stored in "dist/".

Didn't try an ember build before linking. Going to unlink and test that now.

cah-danmonroe · 2016-05-04T23:09:22Z

unlinked, it builds (and runs) as well. However, it is still putting all the ember-paper dependancies under the node_modules/ember-paper/node_modules directory.

(thumbs_up)

miguelcobain · 2016-05-05T11:13:31Z

index.js

+      does not have such a hack for the same reason.
+
+      tl;dr - We want the non built scss files, and b/c this dep is only provided via
+      bower, we use this hack. Please change it if you read this and know a better way.


this dep is only provided via bower

Why do you say that? We're not using bower for the angular-material-source dependency.

They build to their dist directory and publish that to bower only. Their package.json has no main property and the project has no index.js file so tl;dr - there is no way to require their library from another node module - which after looking at some other addons like liquid fire and the node docs, would be our first choice for finding the path for all npm + node versions cleanly via 'path.dirname(require.resolve("angular-material-source"))/src'

This is like the rest of the angular microlibs - they chose to use bower only for deps that used by angular

We could file a ticket to add an empty index.js file in their repo but not sure they'd go for it.

Check the node docs for the 'module' module towards the bottom and then try to find any of the termination steps that will resolve module.main in their code base and you'll see what I mean. I found none

And correct were not using bower, we want the actual source scss, but we're having to hack our own way to find it, which is just weird for sure - although currently ember uses the same pattern (until it becomes addon)

(on my phone ATM so let me know if that made sense - wow using mobile is hard on github, can't even see what I just wrote)

Ah, I see now.
If this.nodeModulesPath points to whatever the current dir for node modules is, then I guess we'll always be fine. Is this correct?

Yeah if that comment in the code base is confusing I should change it. Wanted to explain it is a hack in as few words as possible but to really explain it is rather in depth. If ur brain comes up with a one line comment let's use it 👌

Yeah that is the logic.

I don't think it'll go wrong. It's a hack b/c if it were s real node module we could just require it. But it has no export, so they use it just to install deps, not to be installed by others itself.

As a last comment there - if it were installed globally we'd have no clue where to look but node would

Good grief - that's a lie too. It shakes out fine in all cases. Sorry I was up all nite. I think it's good to merge.

Before I sign off for a bit should we leave the comment so we don't forget or remove it?

I'd prefer a slight rewording. The way it is written looks like it is a hack from ember-paper. It's not really our fault. Besides that, the solution is better than what the comments make it sound.

miguelcobain · 2016-05-05T13:00:18Z

$ ember new paper-test
$ cd paper-test
$ ember install eriktrom/ember-paper#fix-npm-2.x-build-error
$ ember s

This worked, so I assume it is fixed.
Thanks.

miguelcobain · 2016-05-05T13:01:53Z

@eriktrom Please explain the reasoning behind your comment so we can update accordingly.

miguelcobain · 2016-05-05T13:53:33Z

Can someone with npm3 run the steps illustrated above and confirm it works?

* master: Add missing trailing EOL in paper-toolbar-test.js. Remove self-closing tags in dummy app sidenav. fix self closing tags Add missing trailing EOL in paper-toolbar.hbs. update toolbars (miguelcobain#367) use double quotes contributing.md: Add comments to coding style. Update changelog.md. Fix npm 2.x build failures for angular-material-source dependency (miguelcobain#364) Toolbar class bindings (miguelcobain#362) md-button class bindings (miguelcobain#365) progress-circular dummy app: Restore previous behavior inadvertently commented out. progress-circular & -linear dummy app: Start/stop progress when route is activated/deactivated. link directly to `ember-route-action` helper (miguelcobain#363) Bump version to `1.0.0-alpha.0`. dummy app: Make version extremely apparent. contributing.md: Document building and deploying dummy app to github.io. Checkbox & switch examples layout (miguelcobain#361) # Conflicts: # package.json

jtferns · 2016-06-08T19:17:40Z

👋 wanted to mention that I was running into issues when getting a new app with the latest ember-paper built (node 6.1.0, npm 3.8.6) in Circle CI due to the angular-material-source not being found during the addon tree building step; my local workaround was to add "angular-material-source": "angular/material#v1.0.5" (basically an unused dependency) to prevent NPM from moving the 1.0.6 dependency to my project's node_modules

repro steps were the following:

create ember app
install ember-paper: ember install miguelcobain/ember-paper
clear node_modules: rm -rf node_modules/
install fresh node modules: npm install
attempt a build: ember build

The Broccoli Plugin: [Funnel: AngularScssFunnel] failed with:
Error: Attempting to watch missing directory: <EMBER-APP-ROOT>/node_modules/ember-paper/node_modules/angular-material-source/src
    at EventEmitter.Watcher_addWatchDir [as addWatchDir] (<EMBER-APP-ROOT>/node_modules/broccoli-sane-watcher/index.js:90:11)
    at <EMBER-APP-ROOT>/node_modules/ember-cli-broccoli/lib/builder.js:95:35
    at lib$rsvp$$internal$$tryCatch (<EMBER-APP-ROOT>/node_modules/rsvp/dist/rsvp.js:493:16)
    at lib$rsvp$$internal$$invokeCallback (<EMBER-APP-ROOT>/node_modules/rsvp/dist/rsvp.js:505:17)
    at <EMBER-APP-ROOT>/node_modules/rsvp/dist/rsvp.js:1001:13
    at lib$rsvp$asap$$flush (<EMBER-APP-ROOT>/node_modules/rsvp/dist/rsvp.js:1198:9)
    at _combinedTickCallback (internal/process/next_tick.js:67:7)
    at process._tickDomainCallback (internal/process/next_tick.js:122:9)

The broccoli plugin was instantiated at:
    at Funnel.Plugin (<EMBER-APP-ROOT>/node_modules/broccoli-plugin/index.js:7:31)
    at new Funnel (<EMBER-APP-ROOT>/node_modules/broccoli-funnel/index.js:44:10)
    at Class.module.exports.treeForStyles (<EMBER-APP-ROOT>/node_modules/ember-paper/index.js:121:28)
    at Class._treeFor (<EMBER-APP-ROOT>/node_modules/ember-cli/lib/models/addon.js:322:31)
    at Class.treeFor (<EMBER-APP-ROOT>/node_modules/ember-cli/lib/models/addon.js:290:19)
    at <EMBER-APP-ROOT>/node_modules/ember-cli/lib/broccoli/ember-app.js:463:20
    at Array.map (native)
    at EmberApp.addonTreesFor (<EMBER-APP-ROOT>/node_modules/ember-cli/lib/broccoli/ember-app.js:461:30)
    at EmberApp.styles (<EMBER-APP-ROOT>/node_modules/ember-cli/lib/broccoli/ember-app.js:1243:25)
    at EmberApp.toArray (<EMBER-APP-ROOT>/node_modules/ember-cli/lib/broccoli/ember-app.js:1565:10)

eriktrom · 2016-06-08T23:21:47Z

Thanks @jtferns. I think I see a way forward here - I'm a bit wrapped up ATM and don't have the issue u mention, although after upgrading to node 6 I reverted back to 5 in a number of repos due to module resolution errors, many of which seem similar to what u mention (at least in nature).

I do believe I got this to work once (before I decided I had things to do instead of upgrade again ATM) - but I recall getting it all working did not come without pain so I'll fix it up soon

Btw - what version of ember-cli, in case that matters when I debug. Thanks.

eriktrom · 2016-06-08T23:40:42Z

Also is this only for circle CI? That would shed light as well. Does this work on ur local machine?

jtferns · 2016-06-08T23:44:39Z

@eriktrom no worries, glad to shed💡 here; I've had this issue in Circle CI and locally (the repro steps above were local and identical to the errors I was getting on Circle CI);

My ember -v output is:

ember-cli: 2.5.1
node: 6.1.0
os: darwin x64

eriktrom · 2016-06-09T00:06:50Z

Gratzi. The problem in that stack trace btw is b/c isDevelopingAddon is true - which makes ur app watch the addons own deps, for live reload when npm linking, something you don't want for a --prod build obviously. (I'll dig deeper later, but that's the gist of it for the curious - And note to self for later :P)

jtferns · 2016-06-09T00:20:05Z

ah, I see; I've had that flag enabled for in-repo addons but not for external ones, good to know! thanks for 🔍 -ing into this 😄

bdmac · 2016-08-22T15:41:07Z

@miguelcobain this fix does not work for us on node v5.1.0. Get the same error @jtferns mentioned above. This happens both to other developers that did NOT run ember install ember-paper (just ran npm install) and when we deploy.

eriktrom · 2016-08-22T16:06:07Z

@bdmac - what version of npm?

Also where in your node modules tree is angular-material-source located - I actually have the same problem now after I re-installed all my modules recently - I'd like to fix it (and npm with ember in general at some point - I've resorted to checking my modules in!)

any info helps, thanks

eriktrom · 2016-08-22T16:20:17Z

For reference - I'm obviously not sure what the real issue is - but what i said earlier in this thread is irrelevant or wrong - it has to do with where npm puts your modules

EDITED (for clarity)

angular-material-source doesn't have an index.js file nor a main property, its not meant to be installed on its own, the angular team pushes the compiled css to bower as their only form of package distribution - this makes fixing this issue difficult - perhaps one escape hatch we could investigate is to look into how broccoli-funnel is looking up the path, and copy it

Another option - if we we're to make an ember-cli addon for angular-material-source, we could fix this issue there by simply putting their codebase into vendor

eriktrom · 2016-08-22T16:51:23Z

One more note, my knowledge of this problem is mostly informed by this doc (npm module resolution algo, worth the read for those interested in helping discuss a solution to this):

https://nodejs.org/api/modules.html#modules_all_together

eriktrom mentioned this pull request May 4, 2016

Latest master branch does not build #349

Closed

DanChadwick added the bug label May 4, 2016

miguelcobain reviewed May 5, 2016
View reviewed changes

miguelcobain merged commit 32fee2c into miguelcobain:master May 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix npm 2.x build failures for angular-material-source dependency #364

Fix npm 2.x build failures for angular-material-source dependency #364

eriktrom commented May 4, 2016

eriktrom commented May 4, 2016

DanChadwick commented May 4, 2016

eriktrom commented May 4, 2016

cah-danmonroe commented May 4, 2016 •

edited by DanChadwick

cah-danmonroe commented May 4, 2016

cah-danmonroe commented May 4, 2016

miguelcobain May 5, 2016

eriktrom May 5, 2016

miguelcobain May 5, 2016

eriktrom May 5, 2016

eriktrom May 5, 2016

eriktrom May 5, 2016

eriktrom May 5, 2016

eriktrom May 5, 2016

eriktrom May 5, 2016

miguelcobain May 5, 2016

miguelcobain commented May 5, 2016

miguelcobain commented May 5, 2016

miguelcobain commented May 5, 2016

jtferns commented Jun 8, 2016

eriktrom commented Jun 8, 2016

eriktrom commented Jun 8, 2016

jtferns commented Jun 8, 2016

eriktrom commented Jun 9, 2016

jtferns commented Jun 9, 2016

bdmac commented Aug 22, 2016

eriktrom commented Aug 22, 2016 •

edited

eriktrom commented Aug 22, 2016 •

edited

eriktrom commented Aug 22, 2016

Fix npm 2.x build failures for angular-material-source dependency #364

Fix npm 2.x build failures for angular-material-source dependency #364

Conversation

eriktrom commented May 4, 2016

eriktrom commented May 4, 2016

DanChadwick commented May 4, 2016

eriktrom commented May 4, 2016

cah-danmonroe commented May 4, 2016 • edited by DanChadwick

cah-danmonroe commented May 4, 2016

cah-danmonroe commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelcobain commented May 5, 2016

miguelcobain commented May 5, 2016

miguelcobain commented May 5, 2016

jtferns commented Jun 8, 2016

eriktrom commented Jun 8, 2016

eriktrom commented Jun 8, 2016

jtferns commented Jun 8, 2016

eriktrom commented Jun 9, 2016

jtferns commented Jun 9, 2016

bdmac commented Aug 22, 2016

eriktrom commented Aug 22, 2016 • edited

eriktrom commented Aug 22, 2016 • edited

eriktrom commented Aug 22, 2016

cah-danmonroe commented May 4, 2016 •

edited by DanChadwick

eriktrom commented Aug 22, 2016 •

edited

eriktrom commented Aug 22, 2016 •

edited