-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Refactor loading and archiving or scripts post #1059 #1089
Comments
I'm against a In my head, if we stop relying on the OS-dependent That said, I could also be persuaded that using absolute Windows file paths (e.g. |
Something else that would be nice is to remove the archive code from the main |
I would like to recommend on looking into
At least on showing a warning that using absolute paths is not a good idea, and if not used with |
Yeah, showing a warning is probably a good idea 👍 Though if we can show a warning for Windows absolute paths like |
No, because absolute path are platform dependant |
part of #1089 Co-authored-by: na-- <n@andreev.sh>
I am closing this issue - most of the issues here have not been hit by most users. I have never seen a user using two different cases for hte same import or heard of such a case. |
While #1059 will do a lot of good for loading of scripts and their storage in archives:
file://C:/path/to/somewhere.js
orhttps://example.com/test.js
During the development of #1059 multiple times I found out that I need to make more changes or add more problems to be solved in the future. The current PR is more or less the minimal amount of code that fixes problems without introducing new ones (mostly :(). Along the way two things became apparent:
loader
package should be rewritten in a way that is more encapsulated. @na-- gave a good explanation at what that will probably look like in a commentFor this I add this new issue where all current problems will be listed and a solution to them will be drafted, possibly with some problems being marked as non issues:
utils.js
file with for exampleexample.com/utils.js
andhttps://example.com/utils.js
it will be loaded and ran twice. The problems is even bigger if that package in hosted on github/cdnjs and someone uses the shortcut urls as that will be loaded again ...https://example.com/utils.js
andhttps://eXample.com/utils.js
are different files for k6 and will be loaded and executed separately.Additionally windows is in general case insenstive, although you can make it case sensitive and I was told that macos's can be case insensitive, and I won't be surprised if you can make the same for one of the many filesystems that linux uses. So ... we will have to even check whether the filesystem is or is not case insensitive for the particular path we are using it and than ... do something ?!?
C:\\something
is not recognised as absolute path on linux ... obviously which means that we need to specifically copy the code fromfilepath
in order to when we execute an archive which importsC:\\something
to know that is absolutefile
path and not a url with schemeC
and (possibly) opaque\\something
. This has been done in numerous places asfilepath
also doesn't transform\
to/
on linux ... as ... it is OS dependant. (if the user usesfile://C:/something
that will work as there is no ambiguity)cdnjs.com/library/Faker
this will make a url request findout the current versions ofFaker
and then get the url for last one and download that. Currently we write this ashttps://cdnjs.com/library/Faker
in the archive so that we use that specific version when it is ran (also because that was somewhat how was done previously, so for backwards compatibility). This is similar to howgit.luolix.top/something/else
works but without making the intermediate request. This means that if someone uses the exact url this will download and run the script again.Given the fact that on the k6 site it says
I would expect that the scripts that are written to be executed for k6 should work on both windows and linux and macos and whatever else we manage to compile k6 for. Given this having absolute paths and paths with backward slashes in scripts seems to be ... bad idea. While we might manage to fix all of those problems it will be strange to have backward slashes on linux ... Having forward ones on windows is at least ... supported, even if not the standard (for some unknown to me reason).
Given all of that my main proposal for fixing the above problems is to basically not fix 2 and 3 (apart from maybe lower casing the host always ). We should probably start giving warnings to people who use absolute paths and backward slashes, with I propose moving that functionality behind a flag(s) in a year and completely dropping it. And the warning should say that the an archive produced from a script using backward slashes and absolute paths may not work in the future and may not work in the cloud offering even now.
For 1. I would expect that this will be "fixed" on the next rewrite where we completely remove the
loader
package and write something along the lines of theExecutionEnvironment
that @na-- proposed.Just for the record I haven't found many places where node.js advertises they support absolute paths and practically no mentions of it. In their esm proposal it is specifically mentioned that almost everything is URL and if you want absolute paths you need to use
file://
. We can still support moduleSpecifiers which are absolute in this case ... and we can probably even guarantee that they will work from an archive although I still think it is a bad idea to use them and should still spill a warning :)Alternatively we can copy the windows
filepath
code and start calling it even on windows for some scripts ... maybe if we run it from an archive that was made on windows. But then there is difference between running from an archive and having the code on the filesystem ... We can always run all sanitization in an effort to have the same code that work on windows (without absolute paths obviously) to work on linux/macos but if someone made their files with uppercase first lateUtils.js
but reference them withutils.js
that won't work ... I am certain there are more places where this will break ...Another possibility is to ... stop trying to bridge this gap, leave things as are and add a resolveMap(name is up for discussion :)) in order to be able to get the same resolved URL when running from an archive as was resolved when the archive was created.
k6 will record for each moduleSpecifier what it was resolved to (with the pwd it was resolved from) and than write this to the metadata.json. When we run from the archive k6 will use this map to the same resolution. This way archives will be portable, but code will still break if you move them between machines/OSes. The good things about this is that we can write only the REAL final URL (with some sanitizaiton) to the archive and have the map fix the problems with cdnjs/github.com/schemeless urls(/case insensitive filesystems? if we decide that windows is always case insensitive and just lower case everything?)
In this case we will probably have os dependant implementations of the resolving code and possibly even drop support for paths with backward slashes from linux in the future.
The text was updated successfully, but these errors were encountered: