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

Exclude node_modules by default #5210

Merged
merged 1 commit into from Sep 28, 2016

Conversation

Projects
None yet
6 participants
@parkr
Member

parkr commented Aug 7, 2016

If no 'exclude' directive is specified, exclude node_modules by default.

https://twitter.com/mxstbr/status/761856359579185153

/cc @jekyll/build

@mxstbr

This comment has been minimized.

Show comment
Hide comment
@mxstbr

mxstbr Aug 7, 2016

Contributor

No way, this is awesome! 🙌 Glad to have inspired a positive change in Jekyll!

Contributor

mxstbr commented Aug 7, 2016

No way, this is awesome! 🙌 Glad to have inspired a positive change in Jekyll!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 10, 2016

Contributor

If this is gonna happen, can we please also do the same with vendor since it's also a sore spot?

Contributor

envygeeks commented Aug 10, 2016

If this is gonna happen, can we please also do the same with vendor since it's also a sore spot?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 11, 2016

Member

If this is gonna happen, can we please also do the same with vendor since it's also a sore spot?

@envygeeks That's a great idea!

I don't see why we should have defaults like this, people will only ask to make the list bigger. This doesn't affect people in normal usage of Jekyll so there's no need to have it as default, in my opinion.

@spudowiar So many people are using Gulp and Grunt to run Jekyll and confused when it takes like 30s to generate their site. Ultimately, it's a sad result of using NPM that you end up with 15,000 files in a node_modules directory. At least this way we don't unnecessarily slow builds – you basically never want this directory in your resulting site. 😄

Member

parkr commented Aug 11, 2016

If this is gonna happen, can we please also do the same with vendor since it's also a sore spot?

@envygeeks That's a great idea!

I don't see why we should have defaults like this, people will only ask to make the list bigger. This doesn't affect people in normal usage of Jekyll so there's no need to have it as default, in my opinion.

@spudowiar So many people are using Gulp and Grunt to run Jekyll and confused when it takes like 30s to generate their site. Ultimately, it's a sad result of using NPM that you end up with 15,000 files in a node_modules directory. At least this way we don't unnecessarily slow builds – you basically never want this directory in your resulting site. 😄

@parkr parkr added this to the 3.3 milestone Aug 11, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 11, 2016

OK, I see your point. Well vendor and node_modules do seem like sane defaults, just make sure you document it! :)

ghost commented Aug 11, 2016

OK, I see your point. Well vendor and node_modules do seem like sane defaults, just make sure you document it! :)

@Strangehill

This comment has been minimized.

Show comment
Hide comment
@Strangehill

Strangehill Aug 26, 2016

Contributor

Why not add vendor, gemfile, and gemfile.lock into _config.yml?

If no 'exclude' directive is specified, exclude node_modules by default.

will specifying an 'exclude' require node_modules be listed again? Maybe include this too in the exclude list in _config.yml

Contributor

Strangehill commented Aug 26, 2016

Why not add vendor, gemfile, and gemfile.lock into _config.yml?

If no 'exclude' directive is specified, exclude node_modules by default.

will specifying an 'exclude' require node_modules be listed again? Maybe include this too in the exclude list in _config.yml

@Crunch09 Crunch09 referenced this pull request Sep 16, 2016

Merged

Exclude vendor by default #5361

1 of 1 task complete
Exclude node_modules by default
If no 'exclude' directive is specified, exclude node_modules by default.

https://twitter.com/mxstbr/status/761856359579185153
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 16, 2016

Member

This is ready for another round of LGTM's.

Member

parkr commented Sep 16, 2016

This is ready for another round of LGTM's.

@envygeeks

LGTM. It'll solve a lot of user pains.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 28, 2016

Member

@jekyllbot: merge +minor

Member

parkr commented Sep 28, 2016

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 9c197d9 into master Sep 28, 2016

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
jekyll/lgtm Awaiting approval from at least 2 maintainers.
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot deleted the node-must-die branch Sep 28, 2016

jekyllbot added a commit that referenced this pull request Sep 28, 2016

@arechsteiner

This comment has been minimized.

Show comment
Hide comment
@arechsteiner

arechsteiner Oct 19, 2016

@parkr: How to include node_modules now that this change has been released?

I've tried this in my _config.yml:

include: ["node_modules"]

However, the folder node_modules is still excluded.

I'm not very well-versed in Ruby but I think your commit actually forces node_modules to be excluded, instead of making it the default behavior that can be overridden by the user.

arechsteiner commented Oct 19, 2016

@parkr: How to include node_modules now that this change has been released?

I've tried this in my _config.yml:

include: ["node_modules"]

However, the folder node_modules is still excluded.

I'm not very well-versed in Ruby but I think your commit actually forces node_modules to be excluded, instead of making it the default behavior that can be overridden by the user.

@mxstbr

This comment has been minimized.

Show comment
Hide comment
@mxstbr

mxstbr Oct 19, 2016

Contributor

I think just setting exclude to nothing should work:

exclude: []
Contributor

mxstbr commented Oct 19, 2016

I think just setting exclude to nothing should work:

exclude: []
@arechsteiner

This comment has been minimized.

Show comment
Hide comment
@arechsteiner

arechsteiner Oct 19, 2016

Thanks @mxstbr, this works. It's not very intuitive IMHO. Maybe this could be improved some time to honor the include directive and be in line with the documentation:

Include

Force inclusion of directories and/or files in the conversion. .htaccess is a good example since dotfiles are excluded by default.

That's definitely not what happens with this example.

arechsteiner commented Oct 19, 2016

Thanks @mxstbr, this works. It's not very intuitive IMHO. Maybe this could be improved some time to honor the include directive and be in line with the documentation:

Include

Force inclusion of directories and/or files in the conversion. .htaccess is a good example since dotfiles are excluded by default.

That's definitely not what happens with this example.

xueyouchao added a commit to xueyouchao/xueyouchao.github.io-hexo that referenced this pull request Nov 15, 2016

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