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.

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

Jump to bottom

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

Merged
merged 11 commits into from
Nov 15, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
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
6 changes: 6 additions & 0 deletions js/module_loading_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,9 @@ func TestStarImport(t *testing.T) {
r1, err := getSimpleRunner(t, "/script.js", `
import * as cjs from "./commonjs_file.js"; // commonjs
import * as k6 from "k6"; // "new" go module
// go module that only exports default object, but we want it to
// export as a namespace object.
import * as http from "k6/http";
// TODO: test with basic go module maybe

if (cjs.something != 5) {
Expand All @@ -634,6 +637,9 @@ func TestStarImport(t *testing.T) {
if (typeof k6.sleep != "function") {
throw "k6.sleep has wrong type" + typeof k6.sleep;
}
if (typeof http.get != "function") {
throw "http.get has wrong type" + typeof http.get;
}
mstoykov marked this conversation as resolved.
Show resolved Hide resolved
export default () => {}
`, fs, lib.RuntimeOptions{CompatibilityMode: null.StringFrom("extended")})
require.NoError(t, err)
Expand Down
18 changes: 15 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,22 @@ 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

case len(named) == 0 && mi.Exports().Default != nil:
mstoykov marked this conversation as resolved.
Show resolved Hide resolved
// If named length is 0 but default is defined, then try to work with
// default and extract the names of the object's properties. This
// behavior isn't ESM compatible, but we do want to allow defaults to
// be imported as namespaced object, which is also how node works.
if obj, ok := mi.Exports().Default.(*sobek.Object); ok {
gm.exportedNames = obj.GetOwnPropertyNames()
mstoykov marked this conversation as resolved.
Show resolved Hide resolved
}
default:
gm.exportedNames = make([]string, len(named))
mstoykov marked this conversation as resolved.
Show resolved Hide resolved
for name := range named {
gm.exportedNames = append(gm.exportedNames, name)
}
}

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