Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NPM scenario support #26

Closed
wants to merge 9 commits into from
Closed

Conversation

mike-north
Copy link
Member

This may be quick and dirty at first, but I have a need to test 1.13.x >= ember >= 1.7.x, so I'm going to take a stab at it. Let's also let this be a place to start a discussion for "the proper approach" to adding NPM support.

Commands

No changes should be needed to the ember try* commands, although their internals will have to be enhanced

Configuration - config/ember-try.js

////////////////////////
//// EXISTING
////////////////////////

{
  name: 'ember-1.10',
  dependencies: {
    'ember': '~1.10.0'
  },
  resolutions: {
    'ember': '~1.10.0'
  }
}

////////////////////////
//// SUPPORTED
////////////////////////

{
  name: 'ember-1.10',
  dependencies: {
    'ember': '~1.10.0'
  },
  resolutions: {
    'ember': '~1.10.0'
  },
  npm: {
    dependencies: {
      'ember': '~1.10.0'
    },
    resolutions: {
      'ember': '~1.10.0'
    }
  }
}

////////////////////////
//// ENCOURAGED
////////////////////////

{
  name: 'ember-1.10',
  bower: {
    dependencies: {
      'ember': '~1.10.0'
    },
    resolutions: {
      'ember': '~1.10.0'
    },
  }
  npm: {
    dependencies: {
      'ember': '~1.10.0'
    },
    resolutions: {
      'ember': '~1.10.0'
    }
  }
}

Thoughts

  • The cost of a "npm install" is, for most people, way higher than the cost of a bower install.
    • It would be fantastic if adding this feature didn't increase the time it takes to run through a set of scenarios by much.
    • We could probably move the existing node_modules so that it's easy to put back when we want to ember try:reset
    • We probably do not want to blow the whole node_modules folder away if we don't have to
    • There's the issue of ember-try its self being in node_modules.
  • For the use case of dealing w/ various versions of handlebars, some packages will need to be removed from package.json. The current way that bower.json is handled supports only adding.
  • I see no problem with the approach of backing up the bower.json file to bower.json.ember-try. This should apply equally well to package.json
  • Basic implementation
  • Save time by explicitly altering single packages (instead of a broad "npm install")
  • Test coverage for a variety of npm scenarios
  • Documentation

@mike-north
Copy link
Member Author

I'm still just poking around with the code for now, so it's probably not worth reviewing yet :)

@mike-north mike-north changed the title WIP - Proposal & first implementation for npm support Npm support Aug 4, 2015
@mike-north mike-north force-pushed the npm-sloppy branch 2 times, most recently from 2b11072 to 671a154 Compare August 5, 2015 21:19
@mike-north mike-north changed the title Npm support NPM scenario support Aug 5, 2015
@mike-north
Copy link
Member Author

This now works for a bunch of cases that I have tested. Just cleaning stuff up at this point, and potentially logging some more information out so people can feel confident that their scenario is being set up as they expect.


module.exports = {

_npmFoldersToClearForScenario: function (scenario) {
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE TO SELF: Needs a nice comment

@mike-north
Copy link
Member Author

Ok this works now, and is ready for review @kategengler @rwjblue

@@ -21,7 +22,10 @@ module.exports = CoreObject.extend({
//create a fake promise for consistency
promise = RSVP.Promise.resolve();
} else {
promise = BowerHelpers.cleanup(task.project.root);
promise = RSVP.all([
Copy link
Member

Choose a reason for hiding this comment

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

I can't comment on the exact line because github won't let me, but above there's a backupBowerFile call that might need to be accompanied by a backupNpmFile?

@mike-north
Copy link
Member Author

Found a few cases where there are still problems, and I should be able to address them within the next day or two

@ghost
Copy link

ghost commented Aug 21, 2015

Hey @mike-north, whats the status on this?

@mike-north
Copy link
Member Author

Got sidetracked on something else. Not sure when I'll be able to pick this back up, but it needs some more love before merge

@ming-codes
Copy link

👍 This would be very helpful.

Ember 1.13 loads helper automatically, but 1.12 needs another add-on to do this.

@ghost
Copy link

ghost commented Dec 21, 2015

This would be a great feature to have merged, is there any way I can help to get this one merged?

@mike-north
Copy link
Member Author

it proved to be quite a bit more complicated than I initially thought (I
know, surprise!). While my initial solution works in some cases, it's quite
easy to poke holes and make it fall on its face. It's unclear when I'll
have time to give it some love

On Mon, Dec 21, 2015 at 12:54 AM Marten [email protected] wrote:

This would be a great feature to have merged, is there any way I can help
to get this one merged?


Reply to this email directly or view it on GitHub
#26 (comment).

@kategengler
Copy link
Member

I've been working on npm support, but with a very different approach.

@kategengler
Copy link
Member

Closing in favor of #45

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

Successfully merging this pull request may close these issues.

3 participants