-
Notifications
You must be signed in to change notification settings - Fork 365
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
feat: add Netlify Graph support #3904
Conversation
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.
Thank you very much for submitting the PR! Looks already nice overall :) Nevertheless I think you have to rerun the docs command as the docs build is failing. After that try to fix the failing tests and the formatting by running npm test
.
Furthermore please try to add some tests to test the newly added functionality. I know the CLI has not the best testcoverage, but we try to test all the added features as good as possible.
Maybe @KyleBlankRollins can provide some feedback on the documentation if it is ready :)
If you think the PR is not ready for reviewing yet I always tend to move/create a Draft PR on GitHub as the reviewers don't get flooded with mails then :)
5d578c4
to
c7ae2db
Compare
Tests are now failing because snapshots are not matching I guess something changed the behaviour of the dev server will investigate but its actually a code change you introduced @sgrove that is triggering that: |
@sgrove I fixed you the failing tests ;) |
@@ -0,0 +1,278 @@ | |||
/* eslint-disable eslint-comments/disable-enable-pair */ | |||
/* eslint-disable fp/no-loops */ |
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.
opened up discussion regarding this lint rule netlify/eslint-config-node#426
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.
From my point of view it is ready to go 🥳
log, | ||
warn, | ||
error, | ||
debug: console.debug, |
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.
there is a package for debugging
const debug = require('debug')
debug('oneGraph')('My message')
this will only be executed if called with DEBUG=* netlify ...
or DEBUG=oneGraph netlify ....
With that you get fine grained console logs on different debugs scopes
Maybe @ascorbic can provide a last review as well |
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.
🎉
Summary
Basic, preliminary support for Netlify Graph