Skip to content
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

Merged
merged 6 commits into from
Feb 11, 2025

Conversation

TomStrepsil
Copy link

@TomStrepsil TomStrepsil commented Feb 9, 2025

Resolves #1469

Ensure that when babel.loadOptions returns null (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:

    - Expected  - 1
    + Received  + 2
    - import {a} from 'lodash'; a()
    + import { a } from 'lodash';
    + a();

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:

Processed 122 files (1.8s) (1 warning)

✔ No circular dependency found!

✨  Done in 5.72s.
Debugger listening on ws://127.0.0.1:9229/20b17684-51a1-4385-85ce-b6c9202e43ab
For help, see: https://nodejs.org/en/docs/inspector
Starting Danger PR on danger/danger-js#1476
You don't have a DANGER_GITHUB_API_TOKEN set up, this is optional, but TBH, you want to do this
Check out: http://danger.systems/js/guides/the_dangerfile.html#working-on-your-dangerfile
Jest tests passed :+1:

Unable to evaluate the Dangerfile
 Error: process.env.GITHUB_STEP_SUMMARY was not set, which is needed for setSummaryMarkdown
    at Object.setSummaryMarkdown (/danger-js/distribution/platforms/GitHub.js:133:23)
    at dangerfile.ts:89:31
    at step (dangerfile.ts:36:23)
    at Object.next (dangerfile.ts:17:53)
    at fulfilled (dangerfile.ts:8:58)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)


Danger: ⅹ Failing the build, there is 1 fail.
## Failures
Danger failed to run `dangerfile.ts`.
## Markdowns
## Error Error

process.env.GITHUB_STEP_SUMMARY was not set, which is needed for setSummaryMarkdown
Error: process.env.GITHUB_STEP_SUMMARY was not set, which is needed for setSummaryMarkdown
    at Object.setSummaryMarkdown (/git/danger-js/distribution/platforms/GitHub.js:133:23)
    at dangerfile.ts:89:31
    at step (dangerfile.ts:36:23)
    at Object.next (dangerfile.ts:17:53)
    at fulfilled (dangerfile.ts:8:58)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

### Dangerfile

--------------------------^

...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:

node distribution/commands/danger-local.js --base main

Danger: ✓ passed review, received no feedback.

@@ -2,13 +2,15 @@ jest.mock("fs", () => ({
readFileSync: jest.fn(),
realpathSync: {},
existsSync: jest.fn(),
statSync: jest.fn(),
Copy link
Author

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.

@@ -2542,7 +2542,8 @@
"settings": {
"github": {
"accessToken": "12345",
"additionalHeaders": {}
"additionalHeaders": {},
"baseURL": "https://api.github.com"
Copy link
Author

@TomStrepsil TomStrepsil Feb 9, 2025

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.

Copy link
Member

Choose a reason for hiding this comment

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

interesting, weird!

Copy link
Member

@orta orta left a 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!

@orta orta merged commit 3e36bde into danger:main Feb 11, 2025
2 checks passed
@TomStrepsil TomStrepsil deleted the support_babel_ignores branch February 11, 2025 15:53
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.

[BUG] babelify in transpiler assumes no ignored babel files
2 participants