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

refactor: remove error-silencing try/catch on event handler imports #5542

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

voces
Copy link
Contributor

@voces voces commented Apr 18, 2021

Please describe the changes this PR makes and why it should be merged:

This PR removes the try/catch block around requiring event handlers. Before, errors would be silently swallowed, resulting in impacted events unexpectedly not firing. This should only happen due to a bad change or incompatible toolchain (e.g., a bundler that doesn't support dynamic require statements).

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Previous change statement:

Please describe the changes this PR makes and why it should be merged:

This PR changes how action handlers are loaded. Previously, file paths would be resolved via dynamic interpolation with event names (./${name}.js). Some static bundlers, such as esbuild, are unable to resolve these and leave them as-is, which does not work since the bundled file location is going to be different than the source code. Since the require statements were surrounding by try/catch blocks, esbuild would not raise a warning.`

@MattIPv4
Copy link
Contributor

I disagree with this change, it introduces a second place that needs to be updated if events change. I don't really think esbuild is an intended or supported usage of discord.js, and I think the correct thing to do here would be to use a bundler that actually supports all the features of Node.js, including dynamic imports like this...?

@voces
Copy link
Contributor Author

voces commented Apr 22, 2021

Can the try/catch be dropped, though? I don't see why the error should be suppressed, as it represents a real issue and the library is non-operational if it occurs (no events fire). ...And the next person who runs into this same issue won't have to spend nearly as long debugging it.

@uhKevinMC
Copy link
Contributor

uhKevinMC commented Apr 24, 2021

Can the try/catch be dropped, though? I don't see why the error should be suppressed, as it represents a real issue and the library is non-operational if it occurs (no events fire). ...And the next person who runs into this same issue won't have to spend nearly as long debugging it.

I can see where the issue can occur, but this is an internal error that's caused between the Discord API and discord.js, and it's not a simple thing the end-user can do to solve it.

From what I've seen, the try/catch is there to prevent the library from stalling and possibly crashing. There's never been a case where the API can do otherwise to where it makes it this far into the library, but even then there's no reason for anyone to log the errors coming out of that try/catch.

@MattIPv4
Copy link
Contributor

MattIPv4 commented Apr 24, 2021

I can see where the issue can occur, but this is an internal error that's caused between the Discord API and discord.js, and it's not a simple thing the end-user can do to solve it.

That makes literally no sense, what? The code here doesn't touch the API at all, the try/catch is just wrapping dynamic imports that are happening based on a set of defined constants in the library.

I see no reason why this try/catch should ever actually be catching anything (though I don't have full context on why it was added), so imo it makes sense to remove unless it obviously breaks something, but I don't see anything that it could break.

@ChiefChippy2
Copy link

I see no reason why this try/catch should ever actually be catching anything (though I don't have full context on why it was added), so imo it makes sense to remove unless it obviously breaks something, but I don't see anything that it could break.

The code would only error ( and caught silently) if someone manually incorrectly modified/deleted one of the required files. I think it is better to remove the try/catch so people don't get confused why 1 event isn't working or something.

@voces voces changed the title refactor: hardcode action handlers imports refactor: remove error-silencing try/catch on event handler requires Apr 30, 2021
@voces voces changed the title refactor: remove error-silencing try/catch on event handler requires refactor: remove error-silencing try/catch on event handler imports Apr 30, 2021
Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

🤷‍♂️ I don't see why we'd need this, as the handlers are done by us; I guess if you're using a bundler for node (don't.) I can see why this would throw an error so... LGTM but I'm heavily neutral about this change

Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

You shouldn't make web bundles with discord.js but... this try/catch is kind of redundant.

@iCrawl iCrawl merged commit cdcc50f into discordjs:master Apr 30, 2021
@iCrawl iCrawl added this to the Version 13 milestone Jun 27, 2021
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.

9 participants