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

Make require work with HTTPS scheme #1037

Closed
mstoykov opened this issue May 31, 2019 · 1 comment · Fixed by #1059
Closed

Make require work with HTTPS scheme #1037

mstoykov opened this issue May 31, 2019 · 1 comment · Fixed by #1059
Labels
enhancement evaluation needed proposal needs to be validated or tested before fully implementing it in k6 js-compat refactor ux

Comments

@mstoykov
Copy link
Contributor

Currently require() works with urls but ... it is very strange and IMO confusing and error prone.

  1. we specifically don't want any scheme provided which is IMO absurd
  2. we specifically try things that don't start with k6 or / or ./ as urls which leads to confusing messages if you decide that file.js should get you ./file.js. Funilly the messages says that it tried, but as far as I can see it doesn't and if you change it to ./file.js it does work so ... yeah buggy.

My proposal is

  1. make it possible to use https as scheme (possibly file:// as well)
  2. print warnings if we resolve (successfully) a url that wasn't prefaced with https:// - explaining that this will be deprecated. We should also probably change the error message if we try and weren't able to something that also points out that they should probably not do it like this.
  3. wait a few versions/6 months
  4. remove it .
  5. (or 2.5) make it try to resolve local files as if they had ./ in front in case . Dunno if we actually want this or should we just reserve non scheme non ./|/ prefaced imports just for k6.
@mstoykov mstoykov added enhancement ux refactor evaluation needed proposal needs to be validated or tested before fully implementing it in k6 labels May 31, 2019
@na-- na-- added the js-compat label Jun 3, 2019
@mstoykov
Copy link
Contributor Author

mstoykov commented Jun 3, 2019

The specification doesn't really specify how and from where a moduleSpecifier should be loaded, so I am inclined to go with what nodejs is doing (kind of):

  1. have files be required to start with ./ or / (or file://)
  2. if the specified starts with a scheme - and it is one of file:// or https:// we either load it from disk or from the url provided
  3. everything else is considered an internal k6 method ... with the obvious grace period where we just print a warning and otherwise keep the current behaviour of trying to load it from the https:// + moduleSpecifier.
  4. no node_modules like thing ... for now at least

@na-- na-- mentioned this issue Jun 3, 2019
Closed
mstoykov added a commit that referenced this issue Jun 7, 2019
@mstoykov
Refactor loading of modules - support remote modules with scheme
6484f71 
Previously remote modules were supported specifically without scheme.
With this change we specifically prefer if they are with a scheme. The
old variant is supported but prints a warning.
Other changes:
- If remote module requires a relative/absolute path that doesn't have a
scheme it is relative/absolute given the remote module url.
- Support local files with `file` scheme.

fix #1037
@mstoykov mstoykov mentioned this issue Jun 7, 2019
Closed
mstoykov added a commit that referenced this issue Jun 26, 2019
@mstoykov
Rewrite script/files loading to be be url based
aa4b090 
This includes also change to the archive file structure. Now instead of
separating files by whether they are scripts or no, they are separated
based on whether their URI scheme.

Through the (majority) of k6 now instead of simple strings a url.URL is
used to idenitify files(scripts or otherwise).

This also means that all imports or `open` can have schemes.
Previously remote modules were supported specifically without scheme.
With this change we specifically prefer if they are with a scheme. The
old variant is supported but logs a warning.
Additionally if remote module requires a relative/absolute path that doesn't have a
scheme it is relative/absolute given the remote module url.

Because of some of the changes, now caching is done through a afero.Fs
instead of additional map. This also fixes not laoding remotely imported
files from an archive, but instead requesting them again.

fixes #1037, closes #838, fixes #887 and fixes #1051
@mstoykov mstoykov mentioned this issue Jun 26, 2019
Merged
mstoykov added a commit that referenced this issue Jul 31, 2019
@mstoykov
Rewrite script/files loading to be be url based (#1059)
ea8384f 
This also includes a change to the archive file structure. Now instead of
separating files by whether they are scripts or not, they are separated
based on their URI scheme.

Throughout the (majority) of k6 now, instead of simple strings, a url.URL is
used to identify files (scripts or otherwise).
This also means that all imports can have schemes. Previously remote modules
were supported specifically without a scheme. With this change k6 prefers if
they are with a scheme. The old variant is supported, but logs a warning.
Additionally if remote module requires a relative/absolute path that doesn't
have a scheme, it is relative/absolute given the remote module url.

Because of some of the changes, now caching is done through a afero.Fs
instead of additional maps. This also fixes the remotely imported files not 
properly loading from an archive, but instead requesting them again. This is
supported with old archives and for github/cdnjs's shortcut loaders. 

For future-proof reasons the archive now also records the GOOS value of the 
k6 that generated it. 

Anonymize case insensitively as windows paths are (usually) case insensitive. 

fixes #1037, closes #838, fixes #887 and fixes #1051
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement evaluation needed proposal needs to be validated or tested before fully implementing it in k6 js-compat refactor ux
Projects
None yet
Milestone
No milestone
2 participants
@mstoykov @na--