Prefer packages from package.json #12

Closed
mgol opened this Issue Mar 24, 2014 · 15 comments

Comments

Projects
None yet
5 participants
@mgol

mgol commented Mar 24, 2014

Currently jit-grunt first checks for grunt-contrib-package and then for grunt-package in the node_modules directory. I have a branch in my project switching from grunt-contrib-sass to grunt-sass. When I switch between branches I don't want or need to delete the other version as normally it's not loaded/used etc. However, jit-grunt first checks for grunt-contrib-sass, executes it & breaks because my config on this branch is targeted at grunt-sass.

jit-grunt should first check packages declared in package.json and only if no matching ones are found it should fall back to checking all packages present in node_modules.

@jrencz

This comment has been minimized.

Show comment
Hide comment

jrencz commented Mar 24, 2014

+1

@henrahmagix

This comment has been minimized.

Show comment
Hide comment
@henrahmagix

henrahmagix Mar 24, 2014

I think that's too much for jit-grunt to assume. Instead, have a git post-checkout hook reset your node_modules directory.

# .git/hooks/post-checkout
prevHEAD=$1
newHEAD=$2
checkoutType=$3
[[ $checkoutType == 1 ]] && checkoutType='branch' || checkoutType='file';

if [[ $checkoutType == 'branch' ]]
then
    rm -rf node_modules
    npm install
fi

For more on git hooks, see http://stackoverflow.com/a/20892987/3150057

I think that's too much for jit-grunt to assume. Instead, have a git post-checkout hook reset your node_modules directory.

# .git/hooks/post-checkout
prevHEAD=$1
newHEAD=$2
checkoutType=$3
[[ $checkoutType == 1 ]] && checkoutType='branch' || checkoutType='file';

if [[ $checkoutType == 'branch' ]]
then
    rm -rf node_modules
    npm install
fi

For more on git hooks, see http://stackoverflow.com/a/20892987/3150057

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 24, 2014

This would take too long, I want to be able to switch between branches quickly.

Why is it too much to assume? This is only about the order in which packages are checked.

mgol commented Mar 24, 2014

This would take too long, I want to be able to switch between branches quickly.

Why is it too much to assume? This is only about the order in which packages are checked.

@henrahmagix

This comment has been minimized.

Show comment
Hide comment
@henrahmagix

henrahmagix Mar 24, 2014

Or you could create a static mapping: require('jit-grunt')(grunt, {'sass', 'grunt-sass'});

But perhaps you're right: package.json should already be available, and reducing the paths to be checked based on the dependencies would be possible.

Or you could create a static mapping: require('jit-grunt')(grunt, {'sass', 'grunt-sass'});

But perhaps you're right: package.json should already be available, and reducing the paths to be checked based on the dependencies would be possible.

@shootaroo

This comment has been minimized.

Show comment
Hide comment
@shootaroo

shootaroo Mar 25, 2014

Owner

I think this is rare case. Please use static mapping,
or uninstall only grunt-contrib-sass before npm install:

npm uninstall grunt-contrib-sass; npm install

I think npm install will be needed anyway if changed the branch, because version of the grunt plugins that is required will also change, etc.

Owner

shootaroo commented Mar 25, 2014

I think this is rare case. Please use static mapping,
or uninstall only grunt-contrib-sass before npm install:

npm uninstall grunt-contrib-sass; npm install

I think npm install will be needed anyway if changed the branch, because version of the grunt plugins that is required will also change, etc.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 25, 2014

Not necessarily. If I work on switching one of the plugins to a different one or do any other small changes, almost all dependencies will stay the same. This is exactly my use case.

Besides, if you are used to just doing npm install if a problem arises, you will be fine in 99% cases but not here. I'm sure other people using this project will stumble into problems after the switch unless I notify all of them they have to manually uninstall a package (which is less than ideal, not all of them will read the message etc.) or use a static mapping which is not really needed (to be consistent I'd have to define static mappings for all other ~20 packages that normally I don't have to).

mgol commented Mar 25, 2014

Not necessarily. If I work on switching one of the plugins to a different one or do any other small changes, almost all dependencies will stay the same. This is exactly my use case.

Besides, if you are used to just doing npm install if a problem arises, you will be fine in 99% cases but not here. I'm sure other people using this project will stumble into problems after the switch unless I notify all of them they have to manually uninstall a package (which is less than ideal, not all of them will read the message etc.) or use a static mapping which is not really needed (to be consistent I'd have to define static mappings for all other ~20 packages that normally I don't have to).

@henrahmagix

This comment has been minimized.

Show comment
Hide comment
@henrahmagix

henrahmagix Mar 25, 2014

@mzgol I think the main point here is that your development environment should match the branch you're working on. In your case, having grunt-contrib-sass and grunt-sass installed alongside each other is causing you a problem that is solved by refreshing your development environment. It's not jit-grunt's task to assume your environment has other stuff in you want to ignore.

An alternative solution is to add grunt.loadNpmTasks('grunt-contrib-sass) and grunt.loadNpmTasks('grunt-sass) to your branches to distinguish them. jit-grunt won't bother loading a sass grunt task because it will already be available.

@mzgol I think the main point here is that your development environment should match the branch you're working on. In your case, having grunt-contrib-sass and grunt-sass installed alongside each other is causing you a problem that is solved by refreshing your development environment. It's not jit-grunt's task to assume your environment has other stuff in you want to ignore.

An alternative solution is to add grunt.loadNpmTasks('grunt-contrib-sass) and grunt.loadNpmTasks('grunt-sass) to your branches to distinguish them. jit-grunt won't bother loading a sass grunt task because it will already be available.

@shootaroo

This comment has been minimized.

Show comment
Hide comment
@shootaroo

shootaroo Mar 26, 2014

Owner

@mzgol I wrote a "rare case", but was means "exceptional case".
To be used by switching multiple plugins that use the same task name, this is a rare (exceptional) case.

(to be consistent I'd have to define static mappings for all other ~20 packages that normally I don't have to)

Has become a problem only that the grunt-contrib-sass is being used instead of grunt-sass. Add only static mapping to the grunt-sass.

require('jit-grunt')(grunt, {
    sass: 'grunt-sass'
});

Or PR please. I will merging if that is fast enough and simple.

Owner

shootaroo commented Mar 26, 2014

@mzgol I wrote a "rare case", but was means "exceptional case".
To be used by switching multiple plugins that use the same task name, this is a rare (exceptional) case.

(to be consistent I'd have to define static mappings for all other ~20 packages that normally I don't have to)

Has become a problem only that the grunt-contrib-sass is being used instead of grunt-sass. Add only static mapping to the grunt-sass.

require('jit-grunt')(grunt, {
    sass: 'grunt-sass'
});

Or PR please. I will merging if that is fast enough and simple.

@henrahmagix

This comment has been minimized.

Show comment
Hide comment
@henrahmagix

henrahmagix Mar 26, 2014

I think a PR fix for this will cause more problems, e.g. what about tasks that aren't listed in package.json? That file shouldn't be the decider of what tasks jit-grunt should find.

I think a PR fix for this will cause more problems, e.g. what about tasks that aren't listed in package.json? That file shouldn't be the decider of what tasks jit-grunt should find.

@shootaroo

This comment has been minimized.

Show comment
Hide comment
@shootaroo

shootaroo Mar 26, 2014

Owner

@henrahmagix Yes, of course. Resolution of that point is also a condition of the merge.

Owner

shootaroo commented Mar 26, 2014

@henrahmagix Yes, of course. Resolution of that point is also a condition of the merge.

@henrahmagix

This comment has been minimized.

Show comment
Hide comment

👍 =D

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 26, 2014

I think a PR fix for this will cause more problems, e.g. what about tasks that aren't listed in package.json? That file shouldn't be the decider of what tasks jit-grunt should find.

Please read my report again. I explicitly stated that in the absence of the package in package.json the code should fallback to the current lookup algorithm.

I'll submit a PR soon.

mgol commented Mar 26, 2014

I think a PR fix for this will cause more problems, e.g. what about tasks that aren't listed in package.json? That file shouldn't be the decider of what tasks jit-grunt should find.

Please read my report again. I explicitly stated that in the absence of the package in package.json the code should fallback to the current lookup algorithm.

I'll submit a PR soon.

@henrahmagix

This comment has been minimized.

Show comment
Hide comment
@henrahmagix

henrahmagix Mar 26, 2014

Oh yeah, sorry about that =)

Oh yeah, sorry about that =)

@paazmaya

This comment has been minimized.

Show comment
Hide comment
@paazmaya

paazmaya Aug 23, 2014

I had the exact same need, but ended up moving the node_modules/grunt-*sass directory outside of the scope depending on which one I wanted to use.

I had the exact same need, but ended up moving the node_modules/grunt-*sass directory outside of the scope depending on which one I wanted to use.

@shootaroo

This comment has been minimized.

Show comment
Hide comment
@shootaroo

shootaroo May 19, 2015

Owner

Closing due to inactivity.

Owner

shootaroo commented May 19, 2015

Closing due to inactivity.

@shootaroo shootaroo closed this May 19, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment