Introduce esbuild so we have a CommonJS export

[mattwynne]

It turns out that #569 left people unable to load the app, since Probot seems to assume your code is CommonJS.

This is one possible fix. I would prefer if if there's a way we can figure out to have Probot be happy to load our ESM code directly, but I can't see it yet.

[vercel]

@mattwynne is attempting to deploy a commit to the Probot Team on Vercel.

A member of the Team first needs to authorize it.

[mattwynne]

Ref: probot/probot#1684 and probot/probot#1685

[nitrocode]

Thanks @mattwynne for submitting this follow up PR.

@travi can we get another review on this ? Currently probot/settings is broken for us unless we use a self-hosted version of an earlier commit.

[nitrocode]

Could we also look into this version ? It seems like v12 is incorrect. This app must be using at least v16.

https://github.com/probot/settings/blob/master/.nvmrc

GitHub

settings/.nvmrc at master · probot/settings

Pull Requests for GitHub repository settings. Contribute to probot/settings development by creating an account on GitHub.

[travi]

sorry for the delayed response here. i'm glad that you currently have the ability to work around this by hosting at an earlier commit until we get this resolved.

i'm not sure that the additional complexity is worth it to build the app back to cjs after moving the source to esm. i think i lean more in the direction of reverting the esm change until probot is ready to support it. as much as i like moving forward in this direction, i think it would be better to defer the complexity for now. what are your thoughts around that approach @mattwynne? we would obviously still have the esm version available in history so we could bring it back once it is better supported.

[travi]

Could we also look into this version ? It seems like v12 is incorrect. This app must be using at least v16.

thanks for highlighting that. we should definitely update that to v16 at this point. i normally use https://github.com/actions/setup-node/blob/ea3459bb459f783f186dcfc5229793d2c6f40b25/docs/advanced-usage.md#node-version-file at https://github.com/probot/settings/blob/dd947f598604640190b68d75e998fff1bdff4f27/.github/workflows/node-ci.yml#L19 along with the .nvmrc, but overlooked that in this project. would you be interested in contributing a PR with those two changes for us @nitrocode?

GitHub

setup-node/advanced-usage.md at ea3459bb459f783f186dcfc5229793d2c6f40b25 · actions/setup-node

Set up your GitHub Actions workflow with a specific version of node.js - setup-node/advanced-usage.md at ea3459bb459f783f186dcfc5229793d2c6f40b25 · actions/setup-node

[nitrocode]

To get this in a working state again, I think we should revert the last PR commit 9fb82c4 unless we can know this PR will resolve the current issue. Tests are failing in this PR.

With regard to v16 nvmrc, I put in the tiny PR #586 :)

[nitrocode]

We may want to expand on the current test suite to also run the current build to ensure it will start up. That would help future PRs too.

[mattwynne]

sorry for the delayed response here. i'm glad that you currently have the ability to work around this by hosting at an earlier commit until we get this resolved.

i'm not sure that the additional complexity is worth it to build the app back to cjs after moving the source to esm. i think i lean more in the direction of reverting the esm change until probot is ready to support it. as much as i like moving forward in this direction, i think it would be better to defer the complexity for now. what are your thoughts around that approach @mattwynne? we would obviously still have the esm version available in history so we could bring it back once it is better supported.

Honestly, my preference would be to move the codebase to Typescript and then we can compile it down to whatever form of Javascript we like 😁 but I realise that's probably a separate discussion

I'm not married to these changes but I think we should accept that the old commit will be pretty useless once the codebase has moved on. It only took a few hours though so we could easily do it again.

I'm easy either way.

[nitrocode]

Ok, I put in PR #587 to revert PR #569

[travi]

sorry. i did indent to get this taken care of over the weekend, so thanks for the nudge @nitrocode

[travi]

closing this PR since we reverted the ESM change. thanks for digging into the ESM change and this workaround. i hope it will make sense to resurrect some of the pieces soon if probot ends up supporting things better.

[mattwynne]

We may want to expand on the current test suite to also run the current build to ensure it will start up. That would help future PRs too.

I think that would be really helpful

[travi]

i agree that it would be ideal if we could find a way to verify that automatically. i'm hopeful that once we switch over to vercel for hosting that the preview builds for PRs can help with this.