js: refactor how modules are loaded #2881
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2881 +/- ##
==========================================
- Coverage 76.92% 76.84% -0.09%
==========================================
Files 225 229 +4
Lines 16867 16923 +56
==========================================
+ Hits 12975 13004 +29
- Misses 3061 3076 +15
- Partials 831 843 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
mstoykov
force-pushed
the
refactorJS
branch
2 times, most recently
from
e0ec3fd to
8d38091
Compare
| if err != nil { | ||
| return nil, err | ||
| } | ||
| r.pwd = loader.Dir(fileURL) |
There was a problem hiding this comment.
Help me understand this hack... 😅 I read through and understand #2674, just not why we're doing this.
So you store the current requireImpl.pwd so you can revert it in the defer, but why do we need to change it here? The return below calls r.modules.Require() with currentPWD, so it doesn't seem the new WD is used(?).
In general, I'm not clear why we have a global reference to a working directory at all that needs to be changed. Couldn't each imported module have its own pwd, and we can resolve the paths to modules they import using the parent module's pwd? 😕 This way we could avoid keeping and reverting global state.
But I don't have the full picture here, so would appreciate some explanation.
There was a problem hiding this comment.
Couldn't each imported module have its own pwd
No because that will be the breaking change that I describe in #2674 ;)
Or at least we could, and I would argue we should, but not in this PR.
This code as well as practically all the module resolving and loading is just broken down the previous code.
You can see more or less this exact code in the previous version:
Lines 186 to 187 in b750e20
This code specifically does one thing and that is to make the open and require relative to the file that is currently being evaluated within require. Which is different from the file it is in which is basically what you are proposing and how require is supposed to work under commonJS.
How this currently works depend on where it is called from and when and not on just where it is written, Example:
file /path/to/B/B.js:
exports.load = func(s) {
//do somethign cool
require(s); // by commonjs standard this is *always* relative to /path/to/B/B.js
// but in our case it is relative to whatever
// do somethign else cool
}we also have file /path/to/C/C.js which contains are not relevant
file /path/to/A.js has
let s = require("./B/B.js"); // I will be using `require` instead of ESM for the purpose of being simpler ;)
s.load("./C/C.js"); // by the standard this will require `/path/to/B/C/C.js` but k6 has always inturpretted this as `/path/to/C/C.js`This is likely more heavily used with open where you might want to do something in particular.
So in order for this to work require (and open) need to figure out which is the currently being "required" module, and is there could be cycles, and 1 module can be required multiple times I don't think there is another way to do it that isn't stack based which is how I would propose we do it in a future PR.
If you want to try at it or have a specific proposal - I am all ears. But this is kept this way specifically for this to not be a breaking change and so it does have minimal chance of breaking anything - as it works more or less exactly as before.
I do expect another problem is that ... the way this works in k6 is confusing and almost the exact way you expect it to work or should work by how the specification is written.
Functions similar to open for example seem to always work relative to the root script/module and not based on the file it is called in.
Also with connected to #2881 (comment) : this is the code of the call of require but this change to the pwd is for the require or open calls after this one. This require call uses the currentPWD specifically so that it can know what it is relative to.
There was a problem hiding this comment.
I see, thanks for explaining.
I see now that this is what we were doing before, so agree with not changing the behavior in this PR.
I don't have a specific proposal in mind, so let's keep this as is.
|
I have also opened #2900 for the stack based implementation of
That PR is WIP and is only for ... illustrative purposes at this point :) |
This refactor tries to simplify the implementation of `require` and connected code by splitting it heavily into different parts. These changes are similar to what will be needed for native ESM support as shown in #2563 , but without any of the native parts and without doing anything that isn't currently needed. This will hopefully make ESM PR much smaller and less intrusive. This includes still keeping the wrong relativity of `require` as explained in #2674. It also tries to simplify connected code, but due to this being very sensitive code and the changes already being quite big, this is done only to an extent. The lack of new tests is mostly due to there not being really any new code and the tests that were created along these changes already being merged months ago with #2782. Future changes will try to address the above as well as potentially moving the whole module types and logic in separate package to be reused in tests.
| ) | ||
|
|
||
| type module interface { | ||
| Instantiate(vu modules.VU) moduleInstance |
There was a problem hiding this comment.
Why is this method exported? It is not used outside of the js package, right?
There was a problem hiding this comment.
I don't remember - I did write 99% of this code in December ;) and then needed to fix 1 thing.
But in general all of this module and moduleInstance interfaces will be replaced with goja ones, once we go to use ESM, so they will need to be uppercase - they also will have kind of different names - but will be exported. (For the record the goja API is very likely to change as some of the later added features made it way too cumbersome, but there are more ... urgent fixes I need to do like, finalizing merging the current goja master)
Also, as mentioned in a different comment, I want to rip the whole modules system out at some point (preferably before ESM) in a separate package and make it usable for tests including tc39. So if anything I can make the other methods uppercase as well. This likely will be needed to be done later either way 🤷
This refactor tries to simplify the implementation of
requireandconnected code by splitting it heavily into different parts.
These changes are similar to what will be needed for native ESM support
as shown in #2563 , but without any of
the native parts and without doing anything that isn't currently needed.
This will hopefully make ESM PR much smaller and less intrusive.
This includes still keeping the wrong relativity of
requireasexplained in #2674.
It also tries to simplify connected code, but due to this being very
sensitive code and the changes already being quite big, this is done
only to an extent.
The lack of new tests is mostly due to there not being really any new
code and the tests that were created along these changes already being
merged months ago with #2782.
Future changes will try to address the above as well as potentially
moving the whole module types and logic in separate package to be reused
in tests.