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

feat: named exports for esm users #178

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vaporoxx
Copy link
Contributor

@vaporoxx vaporoxx commented Dec 13, 2020

This PR modifies the way index.js re-exports everything so that ESM users can use named exports too. For reference, discord.js applied this exact change with discordjs/discord.js#6884.

Before, this resulted in the following:

import { CommandHandler } from 'discord-akairo';
         ^^^^^^^^^^^^^^
SyntaxError: Named export 'CommandHandler' not found. The requested module 'discord-akairo'
is a CommonJS module, which may not support all module.exports as named exports.

Resolves #122.

@GalGreenfield
Copy link

Looks great! I added it manually to my local installation, thank you!

@GalGreenfield
Copy link

Did you test this? 'cause it didn't work for me.

This doesn't support ESM completely - it does add exports to the various core files, since they, themeslves, have a dependency on various files, they all need to be ESM modules, themeslves, and therefore have to be renamed to .mjs to run on Node.js, and they need to have ES6 import/export syntax, per ESM specifications.

I'm working on converting the whole library's modules to ESM, with the addition of your exports.

@vaporoxx
Copy link
Contributor Author

If you mean loading modules from your own files, that won't be implemented anyway (see #122 (comment)).

As a workaround, i'm loading all modules manually:

async loadModules() {
    for (const file of CommandHandler.readdirRecursive(this.commandHandler.directory)) {
        const { default: module } = await import(`../../${file}`);
        this.commandHandler.load(module);
    }

    for (const file of ListenerHandler.readdirRecursive(this.listenerHandler.directory)) {
        const { default: module } = await import(`../../${file}`);
        this.listenerHandler.load(module);
    }
}

@GalGreenfield
Copy link

GalGreenfield commented Dec 29, 2020

The comment says:

import does not support reloading as of the moment. Its not a deal breaker, but I'd rather not touch it for now.

I don't understand - why and how is reloading of user-defined Command modules necessary?

Also, where do you use loadModules() - in your client?

@vaporoxx
Copy link
Contributor Author

vaporoxx commented Dec 29, 2020

I don't understand - why and how is reloading of user-defined Command modules necessary?

¯\_(ツ)_/¯

Also, where do you use loadModules() - in your client?

I added it as a method to my client class, doesn't matter where/how you do it though

@GalGreenfield
Copy link

I don't understand - why and how is reloading of user-defined Command modules necessary?

¯_(ツ)_/¯

Also, where do you use loadModules() - in your client?

I added it as a method to my client class, doesn't matter where/how you do it though

Haha, okay. I guess I should ask the 1Computer1, then.
Thanks, that's what I thought I'll test it out and see if it works. 🙂

@GalGreenfield
Copy link

GalGreenfield commented Dec 29, 2020

Also, just to make sure - your main file (index or whatever you called it) file and your client class file are both .mjs files and you're using type: "module" in your package.json, yes? I'm asking to make sure I'm reproducing exactly what you did.

@GalGreenfield
Copy link

GalGreenfield commented Dec 29, 2020

Thanks, I tried it and it works great! :)

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.

akairo does not support ES6 syntax
2 participants