Fixes #3929 Allow exporting functions named "default".

[nicklaswj]

This PR fixes #3929.
This PR adds the keyword defaultto the argument skip_keywords for fn function_from_decl when exporting functions.

[daxpedda]

Missing a changelog entry.

I believe that this should be safe to add. @Liamolucko would you mind double-checking me on this?

[Liamolucko]

I believe that this should be safe to add. @Liamolucko would you mind double-checking me on this?

This doesn't work with --target web, since the default export is already used for init. But it works with all other targets.

I don't particularly like adding target-compatibility caveats like this, but I also can't really imagine a scenario where it's an issue - discovering that you aren't allowed to set the default export on --target web isn't really any weirder than discovering that it got silently renamed to _default.

If we do go ahead with this it'd be nice if the CLI printed a proper error when trying to create a default export on the web target, rather than delaying it until the JS is actually used and throws a 'duplicate default export' error.

[nicklaswj]

If we do go ahead with this it'd be nice if the CLI printed a proper error when trying to create a default export on the web target, rather than delaying it until the JS is actually used and throws a 'duplicate default export' error.

I think that sounds reasonable enough. I'll try to extend my PR with a check for --target weband #[wasm_bindgen(js_name = default)]

[nicklaswj]

I've added a check that throws an error if wasm-bindgen-cli is called with --target web and there exist a exported symbol called "default". I'm notoriously bad at creating good error messages, so feedback is welcome 😆

[nicklaswj]

@daxpedda @Liamolucko Not to be pushy, but is there a chance I can get a new review ?

[daxpedda]

I agree with whats said in #3930 (comment), I didn't know that this can't work with the Web target and I think its not the best idea to introduce such target-incompatible changes.

In hindsight I believe that #2855 should not have simply renamed functions, but instead thrown an error.

So I currently think the way forward is to instead of renaming exports where it would make a semantic difference, export them with the export { _default as 'default' } syntax (inspired by #2855 (comment)) or e.g. in the case of the no-modules target can export as-is. Where this is not possible it should return an error.

Let me know if this doesn't address your problem @nicklaswj.
Apologies for the delay, I was on vacation.

[nicklaswj]

@daxpedda I'm sure it's because my JS knowledge is lacking. However I don't understand the semantic difference between your suggestion and what my most recent additions to my PR does.

I'm willing to do the work, however I'm currently not sure what should be done.

[daxpedda]

The point is to make exporting default possible on all targets.

In this case the issue was that default is a keyword in ES modules, which prevented this PR from making a fix for both targets. My suggested solution is to use the export { _default as 'default' } syntax to work around that limitation for the Web target.

This work around could be applied to other keywords as well, therefore a fix should attempt to craft a general solution.

Let me know if anything else is unclear.

[Liamolucko]

In hindsight I believe that #2855 should not have simply renamed functions, but instead thrown an error.

I agree.

In this case the issue was that default is a keyword in ES modules, which prevented this PR from making a fix for both targets. My suggested solution is to use the export { _default as 'default' } syntax to work around that limitation for the Web target.

The PR already ends up doing that though: more specifically export { default1 as default } (turns out it still works without the quotes). It seems to be because of some old code here that special-cases default (but it seems like it was written to allow importing default, not exporting it).

The issue is that the default export is reserved for init on the web target, and so it can't be used to export anything else. It's also literally just an export named 'default' with a bit of syntax sugar around it, so there's no way to have both a default export and an export named 'default' - they're the same thing.

[nicklaswj]

I do not really have an opinion on how we solve it, however it is blocking me. @Liamolucko, if I understand your latest comment correct, I feel like we haven't really reached a conclusion whether I should change anything in my PR or not - is that correctly understood?

[Liamolucko]

@Liamolucko, if I understand your latest comment correct, I feel like we haven't really reached a conclusion whether I should change anything in my PR or not - is that correctly understood?

Well, there are 2 options:

1. Only error if default is exported on the web target (current state of this PR)
2. Error regardless for uniformity

At this point I'm leaning towards option 1 (how often do people really switch targets anyway?), but I was waiting to see what @daxpedda's thoughts were.

however it is blocking me.

In the meantime, how important is it that the default export comes directly out of wasm-bindgen? Is there anything stopping you from making a JS wrapper that reexports it as default?

[daxpedda]

The issue is that the default export is reserved for init on the web target, and so it can't be used to export anything else. It's also literally just an export named 'default' with a bit of syntax sugar around it, so there's no way to have both a default export and an export named 'default' - they're the same thing.

Well that's unfortunate, and here I thought I found a solution that could work.

Well, there are 2 options:

1. Only error if default is exported on the web target (current state of this PR)
2. Error regardless for uniformity

At this point I'm leaning towards option 1 (how often do people really switch targets anyway?), ...

Yeah I think I'm fine with proceeding with the PR then. A solution that came to mind is to add an attribute to explicitly opt-in to the target incompatibility, but I think that's overkill.

[Liamolucko]

Thanks!