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

Supports bundling with esbuild #43

Closed
wants to merge 2 commits into from
Closed

Supports bundling with esbuild #43

wants to merge 2 commits into from

Conversation

r-token
Copy link
Contributor

@r-token r-token commented Sep 19, 2022

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 a try/catch block so that even if esbuild cannot resolve it, it will not fail.

Closes #42

index.js Outdated Show resolved Hide resolved
@dougwilson
Copy link
Contributor

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.

@r-token
Copy link
Contributor Author

r-token commented Sep 19, 2022

Sure. Here are some steps to recreate:

  1. Install esbuild globally via npm install esbuild -g
  2. From this project, run esbuild index.js --bundle

You should get an error message that says ✘ [ERROR] Could not resolve "emitter" with the file and line number in question.

Here's esbuild's getting started page if you need more info.

@dougwilson
Copy link
Contributor

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

@r-token
Copy link
Contributor Author

r-token commented Sep 21, 2022

Hi @dougwilson, just checking in. Anything else you need from me on this?

@dougwilson
Copy link
Contributor

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.

@dougwilson
Copy link
Contributor

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).

@r-token
Copy link
Contributor Author

r-token commented Sep 26, 2022

@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.

@dougwilson
Copy link
Contributor

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.

@r-token
Copy link
Contributor Author

r-token commented Sep 28, 2022

Hi @dougwilson, what's the latest on this? You're not waiting on anything from me, are you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support bundling with esbuild
2 participants