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

Getting 'TypeError: danger.git.JSONDiffForFile is not a function' #367

Closed
svenmuennich opened this issue Aug 30, 2018 · 26 comments · Fixed by #439
Closed

Getting 'TypeError: danger.git.JSONDiffForFile is not a function' #367

svenmuennich opened this issue Aug 30, 2018 · 26 comments · Fixed by #439

Comments

@svenmuennich
Copy link

When running a dangerfile containing a call to danger.git.JSONDiffForFile() on peril the following error is thrown:

error: UnhandledRejection Error:  TypeError: danger.git.JSONDiffForFile is not a function
	at _callee$ (/app/danger-0.9n2i29890gp.js:62:54)
	at Object.apply (/app/node_modules/vm2/lib/contextify.js:87:36)
	at tryCatch (/app/node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:65:40)
	at Generator.invoke [as _invoke] (/app/node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:303:22)
	at tryCatch (/app/node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:65:40)
	at Generator.prototype.(anonymous function) [as next] (/app/node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:117:21)
	at invoke (/app/node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:155:20)
	at /app/node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:202:11
	at new Promise (<anonymous>)
	at callInvokeWithMethodAndArg (/app/node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:201:16)
/app/out/peril.js:25
throw reason;
^
TypeError: danger.git.JSONDiffForFile is not a function
	at _callee$ (/app/danger-0.9n2i29890gp.js:62:54)
	at Object.apply (/app/node_modules/vm2/lib/contextify.js:87:36)
	at tryCatch (/app/node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:65:40)
	at Generator.invoke [as _invoke] (/app/node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:303:22)
	at Generator.prototype.(anonymous function) [as next] (/app/node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:117:21)
	at tryCatch (/app/node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:65:40)
	at invoke (/app/node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:155:20)
	at /app/node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:202:11
	at new Promise (<anonymous>)
	at callInvokeWithMethodAndArg (/app/node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:201:16)

Here's an example dangerfile that should trigger the error.

import { danger } from 'danger';

(async function () {
    const modifiedComposerFile = danger.git.modified_files.find(file => file === 'composer.json');
    if (modifiedComposerFile) {
        const composerDiff = await danger.git.JSONDiffForFile(modifiedComposerFile);
    }
})();

Running the same dangerfile locally using the danger pr command works without error. We use the current latest commit of master (9b835cb). My local danger version is 3.8.8.

Is there any reason why JSONDiffForFile() would not be available in the object danger.git?

@orta
Copy link
Member

orta commented Aug 30, 2018

Hrm, interesting, offhand - no, we have every PR running:

  const path = options.pathToPackageJSON ? options.pathToPackageJSON : "package.json"
  const packageDiff = await danger.git.JSONDiffForFile(path)

  checkForRelease(packageDiff)
  checkForLockfileDiff(packageDiff)
  checkForTypesInDeps(packageDiff)

via danger-plugin-yarn on the artsy org. What happens when you print out Object.keys(danger.git) in this case?

@svenmuennich
Copy link
Author

The output is

[ 'modified_files', 'created_files', 'deleted_files', 'commits' ]

which looks the fields defined in the GitJSONDSL interface.

@orta
Copy link
Member

orta commented Aug 30, 2018

What event is this coming from? ( wondering about #343 )

@svenmuennich
Copy link
Author

It's a pull_request.synchronize event.

@jtreanor
Copy link
Contributor

I am also seeing this issue, with the following rule:

// Podfile should not reference commit hashes
if (danger.git.modified_files.includes("Podfile")) {
    const diff = await danger.git.diffForFile("Podfile");
    if (/\+[^#]*:commit/.test(diff.added)) {
        warn("Podfile: reference to a commit hash");
    }
}

This is the error I see:

danger.git.diffForFile is not a function
TypeError: danger.git.diffForFile is not a function
    at Object.exports.default (/app/danger-0.523gif8a1eb.ts:40:39)
    at process._tickCallback (internal/process/next_tick.js:68:7)

I will look into this and post back if I find a way to solve or work around it.

@orta
Copy link
Member

orta commented Feb 18, 2019

My gut is that somehow Peril thinks this run is for an event and not a PR - https://github.com/danger/peril/blob/master/api/source/github/events/github_runner.ts#L177-L192

@jtreanor
Copy link
Contributor

That's an interesting thought!

If that is the case shouldn't many more things be breaking? e.g. danger.git.modified_files works fine.

@jtreanor
Copy link
Contributor

Could this be some kind of configuration issue?

The Github App has Read & Write permissions for "Checks", "Issues", "Pull Requests" and "Commit Statuses". Should it also have permission for "Repository contents" or similar?

@orta
Copy link
Member

orta commented Feb 18, 2019

Good guess! Repository contents could be it. This is what I use on Peril Staging. I don't think you need as much (I have no idea what people will want to build yet) but it gives a sense

screen shot 2019-02-18 at 9 59 43 am

screen shot 2019-02-18 at 9 59 45 am

screen shot 2019-02-18 at 9 59 49 am

screen shot 2019-02-18 at 9 59 51 am

@jtreanor
Copy link
Contributor

Thanks, @orta.

I have tried adding just "Repository Contents" and also matching your settings (even though its overkill) and unfortunately it didn't help the issue - I restarted the Heroku app after the changes.

As with @svenmuennich, the event seems to be pull_request.synchronize. Here is the log when it happens:

2019-02-18T15:12:33.936581+00:00 app[web.1]: info:
2019-02-18T15:12:33.936714+00:00 app[web.1]: info: ## pull_request.synchronize on unknown on wordpress-mobile/WordPress-iOS
2019-02-18T15:12:33.937086+00:00 app[web.1]: info:    2 runs needed: wordpress-mobile/peril-settings@org/common.ts and wordpress-mobile/peril-settings@org/ios.ts
2019-02-18T15:12:37.646305+00:00 app[web.1]: Unable to evaluate the Dangerfile
2019-02-18T15:12:37.648992+00:00 app[web.1]: info: [runner] - Commenting, with results:
2019-02-18T15:12:37.648995+00:00 app[web.1]: mds: 1
2019-02-18T15:12:37.648997+00:00 app[web.1]: messages: 0
2019-02-18T15:12:37.648999+00:00 app[web.1]: warns: 2
2019-02-18T15:12:37.649001+00:00 app[web.1]: fails: 1
2019-02-18T15:12:37.649002+00:00 app[web.1]:
2019-02-18T15:12:38.289479+00:00 app[web.1]: Found only messages, passing those to review.
2019-02-18T15:12:39.130312+00:00 app[web.1]: Feedback: https://github.com/wordpress-mobile/WordPress-iOS/pull/10931#issuecomment-459309328

@orta
Copy link
Member

orta commented Feb 18, 2019

Can you verify if danger.github.utils.fileLinks exists inside a dangerfile?

I think it's because this line in an even passes in the JSON DSL, not the full DSL (with functions etc) which would only happen if somehow Peril thinks it has an event

https://github.com/danger/peril/blob/master/api/source/runner/run.ts#L85

@jtreanor
Copy link
Contributor

jtreanor commented Feb 18, 2019

Yes, it does exist. I added these two lines:

console.log(`Object.keys(danger.github.utils): ${Object.keys(danger.github.utils)}`);
console.log(`Object.keys(danger.git): ${Object.keys(danger.git)}`);

And got this output:

Object.keys(danger.github.utils): fileLinks,fileContents,createUpdatedIssueWithID,createOrAddLabel,createOrUpdatePR
Object.keys(danger.git): modified_files,created_files,deleted_files,commits

@jtreanor
Copy link
Contributor

I have managed to work around this issue for my particular case by changing the above rule to:

// Podfile should not reference commit hashes
const podfileContents = await danger.github.utils.fileContents("Podfile");
if (/[^#]*:commit/.test(podfileContents)) {
    warn("Podfile: reference to a commit hash");
}

Since danger.github.utils.fileContents works, I can live with this.

I'll be happy to help track this down though if there is anything I can do.

@orta
Copy link
Member

orta commented Feb 18, 2019

OK, well, I think that knocks that idea out that it's the JSON version of the entire Github + git DSL being used. Tricky tricky, haven't found anything so far

@jtreanor
Copy link
Contributor

jtreanor commented Feb 18, 2019

Yeah, it does seem strange.

Could it be an ES6 problem since those methods are added using export interface GitDSL extends GitJSONDSL in https://github.com/danger/danger-js/blob/master/source/dsl/GitDSL.ts#L90?

I'm probably grasping at straws but its the only idea I can think of. My JS knowledge isn't very deep.

@orta
Copy link
Member

orta commented Feb 18, 2019

No worries, those are the type definitions (think .h files) - which don't really affect runtime behavior. I avoid classes in ES6, this is just confusing.

Somehow the function here isn't getting called:

https://github.com/danger/danger-js/blob/56491b4d8b4adfb08a69dcc087b36f82a05939eb/source/platforms/git/gitJSONToGitDSL.ts#L50-L51

@jtreanor
Copy link
Contributor

Oh, that's interesting. Thanks for the context!

@jtreanor
Copy link
Contributor

jtreanor commented Feb 26, 2019

I'm seeing another very similar issue now that I have updated to master. GitHubDSL does not seem to be loaded, but GitHubJSONDSL is. This means that both danger.github.api and danger.github.utils are undefined.

On master (4c54ab9), console.log(Object.keys(danger.github)); gives:

[ 'issue', 'pr', 'commits', 'reviews', 'requested_reviewers', 'thisPR' ]

On the commit I have reverted to (b0d8bdb), it gives:

[ 'issue', 'pr', 'commits', 'reviews', 'requested_reviewers', 'thisPR', 'api',  'utils' ]

This may be a danger-js issue, which was updated from 6.0.9 . to 7.1.9 between these commits.

Update: I have confirmed that this was introduced when danger was updated to 7.1.9 prior to the mono repo change (0346e66).

@orta
Copy link
Member

orta commented Feb 26, 2019

Interesting!

So the reason I major semver bumped was because it updated the GitHub API client (which could break dangerfiles) but that shouldn't be causing this

@roopakv
Copy link
Member

roopakv commented Jun 21, 2019

@jtreanor I'm trying to get peril setup for our org and running into the same issue you've described here. What commitsha of peril did you end up deploying that gives you access to be able to get files usign danger.github.utils.fileContents?

Want to get something up and running and then can help debug on my own time :)

@jtreanor
Copy link
Contributor

Hi @roopakv,

I went with b0d8bdb. I have tried digging into this a few times but still haven't got to the bottom of it.

@roopakv
Copy link
Member

roopakv commented Jun 21, 2019

thank you! do you happen to have an open copy of hwo you are running async functions with that version. I keep getting unexpected token export and well then peril decides my script is done before the async method returns :(

@jtreanor
Copy link
Contributor

@roopakv All of the Peril settings I am using are in this repo: https://github.com/wordpress-mobile/peril-settings. I hope thats helpful!

@roopakv
Copy link
Member

roopakv commented Jul 7, 2019

Was this by chance also fixed with #436 ?

@jtreanor
Copy link
Contributor

jtreanor commented Jul 8, 2019

@roopakv No, it wasn't unfortunately. I'm currently investigating it.

@jtreanor
Copy link
Contributor

jtreanor commented Jul 9, 2019

I have put together a fix for this in #439.

PR runs were not using the full DSL so I have fixed that.

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 a pull request may close this issue.

4 participants