Skip to content

fix(nvmrc): remove nvmrc checks and reenable node6 support#137

Merged
jeff-phillips-18 merged 1 commit intopatternfly:masterfrom
priley86:remove-nvmrc
Dec 20, 2017
Merged

fix(nvmrc): remove nvmrc checks and reenable node6 support#137
jeff-phillips-18 merged 1 commit intopatternfly:masterfrom
priley86:remove-nvmrc

Conversation

@priley86
Copy link
Member

@priley86 priley86 commented Dec 20, 2017

What:
Closes #136 and removes nvmrc-check

cc: @jeff-phillips-18 @sharvit

@sharvit
Copy link
Contributor

sharvit commented Dec 20, 2017

@priley86
Copy link
Member Author

priley86 commented Dec 20, 2017

so we need Node 8 while developing in this project, but I don't think we want to specify that downstream (you could be using Node6/Node5/Node4) for example there. So I don't think a postinstall check fits the bill...

suggestions on this?

@jeff-phillips-18
Copy link
Member

I'm fine with removing the postinstall check. We likely want to remove the extra commas from :

https://github.com/patternfly/patternfly-react/blob/master/storybook/webpack.config.js#L29 https://github.com/patternfly/patternfly-react/blob/master/storybook/webpack.config.js#L33

and add the file to eslint ignore. Or, better yet, remove the eslint requirement to have trailing commas.

@priley86 priley86 changed the title fix(nvmrc): remove nvmrc checks for now fix(nvmrc): remove nvmrc checks and reenable node6 support Dec 20, 2017
@priley86
Copy link
Member Author

priley86 commented Dec 20, 2017

yep... Node6 seems to break when running npm run storybook currently... 👍

Update the PR to just change that setting globally and build node 6, as I'd prefer not to add ignores and get myself confused 😸

Copy link
Member

@cdcabrera cdcabrera left a comment

Choose a reason for hiding this comment

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

Agreeable on the update, postinstall always seems to come back around, happened on some of the other PF repos with Jekyll, same outcome removing the script.

If the trailing comma stuff pops back up as someone wanting to implement it under the guise of "it affects diffs" an alternative is the leading comma (not a fan of this style).

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.

4 participants