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

Implemented an 'afterExtract' function hook RE: #270 #354

Closed
wants to merge 1 commit into from

Conversation

MarshallOfSound
Copy link
Member

This PR adds an afterExtract option to the API. This option should be an array of functions, each of which is called directly after electron-packager has finished extracting to the temporary directory.

Relevant issue: #270

})
series(newFunctions, cb)
}
})
Copy link
Member

@malept malept Apr 30, 2016

Choose a reason for hiding this comment

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

I think the callback you defined for extract should be another function in the series.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you suggesting then that we should do

series([extract(zipPath, {dir: buildDir})].concat(newFunctions))

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking more along the lines of

function (cb) {
  extract(zipPath, {dir: buildDir}, cb)
},
function (cb) {
  if (opts.afterExtract exists and is not array) {
    return error
  }
  // etc.
}

...that got pseudocode-y, fast.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah got it 👍

Done


* `buildPath` - The path to the temporary folder where Electron has been extracted to
* `electronVersion` - The version of electron you are packaging for
* `callback` - Must be called once you have completed your actions
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit: the rest of the file has standardized on - for list items.

@malept malept added the tests-needed Pull request needs tests label Apr 30, 2016
@malept
Copy link
Member

malept commented Apr 30, 2016

Looks good, I think this just needs at least one unit test.

@MarshallOfSound
Copy link
Member Author

@malept Not too familiar with the testing framework here but I added some tests 👍

@malept
Copy link
Member

malept commented Apr 30, 2016

The test logic looks OK at a glance (I'll look at it in more detail tomorrow), but I'd prefer if it had its own test instead of tacking it onto another test.

@MarshallOfSound
Copy link
Member Author

@malept I've moved the logic to it's own test and moved it to a new file called "hooks" so that if in the future we add more of these hooks we have a place for them to go 👍

@malept
Copy link
Member

malept commented May 25, 2016

Sorry about the delay, work & real life got in the way.

I had merged #351 before this one, so if you could take care of the strict changes in test/hooks.js, I'll merge.

@MarshallOfSound
Copy link
Member Author

@malept Talk about delay 😆

I finally got around to adding use strict 😁

@malept
Copy link
Member

malept commented Jun 15, 2016

Amusingly enough, this morning I did something similar. The only difference is that the require statements are const:
https://github.com/electron-userland/electron-packager/tree/after-extract-hook

@MarshallOfSound
Copy link
Member Author

@malept So who's do we use 😆

@malept malept mentioned this pull request Jun 22, 2016
@malept
Copy link
Member

malept commented Jun 22, 2016

I added a PR for my branch (#403). I'll wait for CI to run and then merge it.

@malept malept closed this in #403 Jun 23, 2016
malept added a commit that referenced this pull request Jun 23, 2016
@malept malept added this to the 7.1.0 milestone Jun 23, 2016
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.

2 participants