-
-
Notifications
You must be signed in to change notification settings - Fork 372
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
Support files being ignored by babel configuration #1476
Conversation
@@ -2,13 +2,15 @@ jest.mock("fs", () => ({ | |||
readFileSync: jest.fn(), | |||
realpathSync: {}, | |||
existsSync: jest.fn(), | |||
statSync: jest.fn(), |
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.
N.B. These are needed to prevent an early-return within babel.
…g release process
@@ -2542,7 +2542,8 @@ | |||
"settings": { | |||
"github": { | |||
"accessToken": "12345", | |||
"additionalHeaders": {} | |||
"additionalHeaders": {}, | |||
"baseURL": "https://api.github.com" |
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.
N.B. This was produced when running yarn lint
- unclear why previous PRs did not produce the same? But without this, the CI fails.
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.
interesting, weird!
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.
Yeah, this makes sense to me 👍🏻
Thanks!
Resolves #1469
Ensure that when
babel.loadOptions
returnsnull
(which will be the case if a file is ignored by a babel configuration), this falls through to the guard to return the minimal options ({ plugins: [] }
), thus preventing the error described in the issue.For context, this was needed in a project that used a local babel preset, which had to be ignored to avoid a circular reference in babel. When using a typescript dangerfile, the shared babel config baulked, due to attempt to spread
null
.This could be mitigated via
DANGER_DISABLE_TRANSPILATION
, but this then requires any local danger plugins etc to be in commonJs, which isn't ideal.N.B. The
babelify
code was previously untested, and I have added bare-minimum test of the new functionality I have added, to simplify the review. However, I am happy to attempt to "scout rule" the remainder of the function in tests, if that will be welcome.N.B. The mocked dangerfile (copied from prior-art in the same test file) had to be changed to a linted version - since despite tests passing locally otherwise, the appveyor build had a failure thus:
p.s. running:
yarn build; node --inspect distribution/commands/danger-pr.js https://github.com/danger/danger-js/pull/1476
...as per the guide, has produced:
...which appears unrelated, and unclear how this can be resolved as a contributor?
If I run danger locally on this branch, no issues are present: