Skip to content

Fix namespaced object export when default is the only one available #4058

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Nov 15, 2024
Merged

Fix namespaced object export when default is the only one available #4058

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
68a2440
Refactor to a switch
ankur22 Nov 14, 2024
efb0611
Add conversion from default to namspaced object
ankur22 Nov 14, 2024
ac530c6
Update test to handle default to namespaced object
ankur22 Nov 14, 2024
241af93
Update to work with nil check over len
ankur22 Nov 14, 2024
134399b
Fix slice make
ankur22 Nov 14, 2024
296f8c2
Update to work with ToObject instead of cast
ankur22 Nov 14, 2024
502376b
Refactor test into a sub test
ankur22 Nov 14, 2024
b301b49
Remove non ESM import from test
ankur22 Nov 14, 2024
f492b40
Add a test to compare object and namespaced obj
ankur22 Nov 14, 2024
37a78c6
Refactor back to if/else instead of switch
ankur22 Nov 15, 2024
636484f
Update comment
ankur22 Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Refactor to a switch 
This allows us to work with a more go way of working with conditional
branching.
  • Loading branch information
@ankur22
ankur22 committed Nov 14, 2024
commit 68a2440eb53368677f9db38570c5fc146f3f05d2
10 changes: 7 additions & 3 deletions js/modules/gomodule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,14 @@ func (gm *goModule) Instantiate(rt *sobek.Runtime) (sobek.CyclicModuleInstance,
if gm.exportedNames == nil {
named := mi.Exports().Named

gm.exportedNames = make([]string, len(named))
for name := range named {
gm.exportedNames = append(gm.exportedNames, name)
switch {
Copy link
Contributor

Choose a reason for hiding this comment

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

I expect you used switch as there were more than one cases earlier?

Copy link
Contributor Author

@ankur22 ankur22 Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do consider it go like in the cases where:

  1. it can't be rewritten as a signel if/else pair - this can
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, happy to hear more feedback, before merging/reverting the switch change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted it back to if/else in 37a78c6

default:
gm.exportedNames = make([]string, len(named))
for name := range named {
gm.exportedNames = append(gm.exportedNames, name)
}
}

for _, callback := range gm.exportedNamesCallbacks {
callback(gm.exportedNames)
}
Expand Down