js: Cache scripts by absolute not relative paths #2903
Conversation
This fixes a regression introduced in 2753196 where in all imports are cached on the specifier that is provided for them. This is fine for k6 modules which happen to always have the same absolute names. But source modules can have relative paths that resolve to different absolute ones depending on where they are imported from. Fixes #2902
mstoykov
added
high prio
breaking change
js/initcontext.go
Outdated
| programs map[string]programWithSource | ||
| exportsCache map[string]goja.Value | ||
| programs map[string]programWithSource | ||
| // mege this and the above |
There was a problem hiding this comment.
| // mege this and the above | |
| // merge this and the above |
Can you document when it is expected to be done and what is the issue at the moment that blocks it, please?
There was a problem hiding this comment.
The blockign part is mostly that thsi will be quite a big refactor which in practice is done in #2881
| if export, ok = i.k6ModulesCache[arg]; ok { | ||
| return export | ||
| } | ||
| defer func() { i.k6ModulesCache[arg] = export }() |
There was a problem hiding this comment.
Could you clarify why the code in defer is required, does it expected to be not nil in some cases?
There was a problem hiding this comment.
This code sets the cache to whatever is returned.
I can rewrite this more I guess moving the whole reference to k6ModulesCache to requireModule.
It was just a lot more easier and faster to just move teh code that did this irregardless of source or go module to be just a few lines below.
There was a problem hiding this comment.
Do you want me to rewrite if we are going to be getting #2881 next release?
Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## master #2903 +/- ##
==========================================
- Coverage 76.88% 76.73% -0.16%
==========================================
Files 225 223 -2
Lines 16867 16865 -2
==========================================
- Hits 12969 12941 -28
- Misses 3066 3086 +20
- Partials 832 838 +6
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. |
This fixes a regression introduced in 2753196 where in all imports are cached on the specifier that is provided for them. This is fine for k6 modules which happen to always have the same absolute names.
But source modules can have relative paths that resolve to different absolute ones depending on where they are imported from.
For example: import { cool } from "./somepath/cool.js"` - was cached based on the name/key "./somepath/cool.js" and it will be returned if anywhere in the code that same import specifier is used.
But this is a bug if this is done from two different directories.
As in one called from
/A/A.jsand from/B/B.jsshould get two different files/A/somepath/cool.jsand/B/somepath/cool.js. Instead, in v0.42.0 the first to load the relative path will be wrongly cached with the relative path, and then it will be used for the other import as well.Now k6 again caches source modules based on their absolute paths.
Fixes #2902