-
Notifications
You must be signed in to change notification settings - Fork 52
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
Supports bundling with esbuild
#43
Conversation
Please also add either a test for this or at least instructions here for how to reproduce the issue so I can create a test for it so this doesn't regress. |
Sure. Here are some steps to recreate:
You should get an error message that says Here's esbuild's getting started page if you need more info. |
Sweet, thanks! I will push up a commit here soon that wraps that in to a test so we don't break it 👍 . Thank you so much and I'll get this merged and released either today or tomorrow :D |
Hi @dougwilson, just checking in. Anything else you need from me on this? |
Hi @r-token thanks for the ping and sorry for being silent on your PR. In the process of adding the test, I noticed this project was not running the CI. I had to message Travis CI to see why it wasn't using my credits and apparently I need to migrate the project. I am just getting that done so the tests will run, as it's a Travis CI step I'm adding to bundle the module and test the bundle. |
Hi @r-token sorry, it looks like I am unable to push the tests to this PR. Usually this is enabled by default, sorry about that. I just need you to check the box on the right side here to enable edits from maintainers (https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork). |
@dougwilson I don't have that checkbox, unfortunately. I think it's because I opened this PR from my organization's account instead of my personal account (isaacs/github#1681). If necessary, I can open a separate PR with the same changes from my personal account. If you have a different/better idea though, I'm all for it. Let me know what you think. |
Ah, interesting, didn't know that. It's ni problem, I can still land everything. No need to reclone move changes and all that 👍. I will nake time tonight for you to get this merged and released. |
Hi @dougwilson, what's the latest on this? You're not waiting on anything from me, are you? |
As discussed in #42, bundling with esbuild currently fails on this package due to an issue on line 15 of index.js. It references a package called "emitter" that esbuild cannot resolve, thus esbuild fails.
This PR simply wraps the
var Emitter = require('emitter');
line in atry/catch
block so that even if esbuild cannot resolve it, it will not fail.Closes #42