Fix namespaced object export when default is the only one available #4058
Conversation
This allows us to work with a more go way of working with conditional branching.
If no named exports are available, we want to try and convert the default export to named exports. This allows users to work with namespaced objects even through the module hasn't been setup to work in that way. This is how node works too.
ankur22
requested review from
inancgumus,
joanlopez and
mstoykov
and removed request for
a team
js/modules/gomodule.go
Outdated
| gm.exportedNames = make([]string, len(named)) | ||
| for name := range named { | ||
| gm.exportedNames = append(gm.exportedNames, name) | ||
| switch { |
There was a problem hiding this comment.
I expect you used switch as there were more than one cases earlier?
There was a problem hiding this comment.
I refactored it since i think it's easier to follow when there are more than one conditions in the same function, instead of a if/else/else if etc. I also believe it's more go like to work with switch instead of if/else/else if.
There was a problem hiding this comment.
I do consider it go like in the cases where:
- it can't be rewritten as a signel if/else pair - this can
- or when using a case can be used to check a single variable against multiple values and that is the only thing happening:
s := "apple";
switch s{
case "cucumber", "pepper", "leek":
return "vegetable";
default:
return "not vegetable"
}As in both of this case it makes it easier to read. I would argue that using a switch in other cases makes me think there is something strange going on.
I will let other people also look at this and see what they think on it - so this is not a request for a change at this point, but a nit.
There was a problem hiding this comment.
Yeah, happy to hear more feedback, before merging/reverting the switch change.
There was a problem hiding this comment.
I've reverted it back to if/else in 37a78c6
This is to stay consistent with other areas of the codebase (e.g. toESModuleExports). There could be a valid case where we want named to be an empty slice and default be non-nil, and not use this feature to convert from default to namespace exports.
It needs to be of length 0 to allow for append from 0.
This is a better and safer way to work with an interface{} type when we
need to retrieve the property values from the object. Since the
underlying type of Default could be anything, including a sobek.Value,
it's better to retrieve it as a sobek.Object instead of a casting.
This will allow us to create other tests under the StarImport domain.
What?
This fixes an issue where a module only expose a default object, but we also want to allow the the module to be imported as a namespaced object.
Why?
This is how node works, and this is how we have chosen to work even after the changes to work with ESM from commonJS. It prevents breaking changes with older scripts that were not intended.
Checklist
make lint) and all checks pass.make tests) and all tests pass.Related PR(s)/Issue(s)
Related: #4050