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

Use readFileSync instead of require #101

Merged
merged 2 commits into from
Aug 30, 2019
Merged

Use readFileSync instead of require #101

merged 2 commits into from
Aug 30, 2019

Conversation

damccorm
Copy link
Contributor

This should simplify webpack configs so we avoid things like #100

@@ -21,7 +20,9 @@ export class Context {
*/
constructor() {
this.payload = process.env.GITHUB_EVENT_PATH
? require(process.env.GITHUB_EVENT_PATH)
? JSON.parse(
readFileSync(process.env.GITHUB_EVENT_PATH, {encoding: 'utf8'})

Choose a reason for hiding this comment

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

Do we want to add error handling here? Make sure the file exists first and print message if not?

Choose a reason for hiding this comment

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

And while we are here, should we debug print a serialization of the context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error handling yes, serialization of the context seems like a no to me. Its pretty massive, and since we're built to be a dependency I'm a little leary of large debug output that may or may not be helpful, especially since lots of actions probably won't use that directly.

@damccorm damccorm merged commit 99d3ad0 into master Aug 30, 2019
@damccorm damccorm deleted the webpack branch August 30, 2019 17:02
clydin added a commit to clydin/dev-infra that referenced this pull request Sep 3, 2019
… `@actions/github` package

The underlying issue will be fixed directly in the next release of the package. actions/toolkit#101
@gnzzz
Copy link

gnzzz commented Sep 3, 2019

What's the release schedule for the @actions/github npm package? I'm currently manually webpacking this change but would much rather use the proper releases.

clydin added a commit to clydin/dev-infra that referenced this pull request Sep 3, 2019
… `@actions/github`

The underlying issue will be fixed directly in the next release of the package. actions/toolkit#101
josephperrott pushed a commit to angular/dev-infra that referenced this pull request Sep 3, 2019
… `@actions/github` (#31)

The underlying issue will be fixed directly in the next release of the package. actions/toolkit#101
@damccorm
Copy link
Contributor Author

damccorm commented Sep 3, 2019

Just published, sorry for the delay - https://www.npmjs.com/package/@actions/github

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.

3 participants