discordjs / voice

Implementation of the Discord Voice API for discord.js and other JS/TS libraries
Apache License 2.0
327 stars 110 forks source link

fix: resolve esm and cjs issue #232

Closed vijayymmeena closed 3 years ago

vijayymmeena commented 3 years ago

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

esm issue raised in #227 and later fixed in #229 but unfortunately the change applied to cjs (#231) as well, tsup banner solution does not provide any configuration that we can use atm. This PR is solution to resolve this problem with custom script.

codecov[bot] commented 3 years ago

Codecov Report

Merging #232 (083e87f) into main (3bfaebf) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #232   +/-   ##
=======================================
  Coverage   73.65%   73.65%           
=======================================
  Files          21       21           
  Lines         911      911           
  Branches      227      227           
=======================================
  Hits          671      671           
  Misses        238      238           
  Partials        2        2           

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update 3bfaebf...083e87f. Read the comment docs.

vijayymmeena commented 3 years ago

there is one more reason, why we could not use banner. The import.meta.url resolved at build time. Which is not a ideal solution.

screenshot of present build output image

vijayymmeena commented 3 years ago

ESM Test Result image

note: cjs tests are not required.

vijayymmeena commented 3 years ago

image

@iCrawl

vijayymmeena commented 3 years ago

my bad sorry

vijayymmeena commented 3 years ago

no it's not

iCrawl commented 3 years ago

Oh, I see, it's the resolve at build time.

vijayymmeena commented 3 years ago

@iCrawl I am not sure, but please resolve this.

image

in node module, the package is updated and I can verify with package.json version. But with generateDependencyReport() is shows 0.7.3 and your changes are there as well.

image

but still issue is not resolved with ESM.

iCrawl commented 3 years ago

Lets try again in 0.7.5 πŸ™ƒ

vijayymmeena commented 3 years ago

Lets try again in 0.7.5 πŸ™ƒ

well that's the solution given in this PR, so no doubt it will work.

ref: https://github.com/discordjs/voice/commit/644af9579f02724c489514f482640b8413d2c305

iCrawl commented 3 years ago

Yeah I just didn't want to bring in another dependency for something so simple.

vijayymmeena commented 3 years ago

Yeah I just didn't want to bring in another dependency for something so simple.

yes, I just didn't think about fs in first place, my bad. Anyway, thanks.