-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Fixes binyamin/eleventy-garden#61 outdated useGitIgnore #62
Conversation
Sorry but can't seem to link the pullrequest to the issue neither by title or manually. Not sure if it's a setting - I'm new to open source/github. |
Thanks for catching this. Your changes look great; I just had one quick question before I merge them (see #62 (comment)). Thanks so much for your time. |
.eleventyignore
Outdated
node_modules/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens without these bottom 2 lines? Aren't they ignored by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm not sure why it's only showing the last line. I meant _site/
and node_modules/
(lines 5-6)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If node_modules is left out it gives an error on npm start (even if stated in the documentation that it should be autoignored). It tries to parse the directory:
> Having trouble rendering liquid template ./node_modules/balanced-match/README.md
`TemplateContentRenderError` was thrown
> unexpected token at "}` will match...", file:./node_modules/balanced-match/README.md, line:50, col:130
`ParseError` was thrown
> unexpected token at "}` will match..."
I can remove _site/ from the ignore - it works without it as long as _site/ is set as outputdirectory. Would you prefer it out of .eleventyignore for simplicity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. do that, then I'll merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Change title back to just #61 if that makes sense - the title change was an attempt to get it to autolink the issue :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that if I up the 11ty version to newest stable (^1.0.0-canary.44) the default works for node_modules and the site still builds and serves without node_modules in .eleventyignore. That seems like an even better solution. Should I add it to this pull request or create a separate one to update to newest version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just merged #54 from RenovateBot, which updated eleventy. Basically, Renovate opens PRs for outdated dependencies.
I could merge now, or you could make that edit to the eleventyignore. I think that using setUseGitIgnore
is a good enough change.
Newer versions of 11ty sets this differently. See documentation at: https://www.11ty.dev/docs/ignores/
Outputfolder and node_modules defaults to ignore
Newer versions of 11ty sets this differently. See documentation at: https://www.11ty.dev/docs/ignores/
Fixes #61