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.

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 of modules - support remote modules with scheme #1046

Closed
wants to merge 2 commits into from

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Jun 7, 2019

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

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 requested a review from na-- June 7, 2019 15:49
loader/loader.go Outdated Show resolved Hide resolved
@mstoykov
Copy link
Contributor Author

mstoykov commented Jun 7, 2019

Things that should probably be fixed:

  • Some of the error messages: specifically when you try to load remote url and it fails make it seem like we tried loading from the fs and than the url and nothing worked which is lie, now and previously.

  • The way k6 archive t.js works in case of https:// urls and probably file:// should be fixed. This is somewhat related to Running from archive still downloads scripts from the internet #887 .

  • Make the loader(github/cdnjs) return the real URLs and put those in the archive ?!? related to the previous one

  • Better test of when url.Parse fails.

@codecov
Copy link

codecov bot commented Jun 7, 2019

Codecov Report

Merging #1046 into master will decrease coverage by 0.02%.
The diff coverage is 73.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1046      +/-   ##
==========================================
- Coverage   72.67%   72.65%   -0.03%     
==========================================
  Files         133      133              
  Lines        9846     9892      +46     
==========================================
+ Hits         7156     7187      +31     
- Misses       2274     2286      +12     
- Partials      416      419       +3
Impacted Files Coverage Δ
loader/loader.go 81.59% <73.01%> (-8.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 607b63c...d961e66. Read the comment docs.

@mstoykov
Copy link
Contributor Author

superseded by #1059

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make require work with HTTPS scheme
2 participants