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

Remove trying to load remote modules by prepending https:// #3287

Closed
mstoykov opened this issue Aug 18, 2023 · 3 comments · Fixed by #3451
Closed

Remove trying to load remote modules by prepending https:// #3287

mstoykov opened this issue Aug 18, 2023 · 3 comments · Fixed by #3451
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes
Milestone

Comments

@mstoykov
Copy link
Collaborator

mstoykov commented Aug 18, 2023

What?

if currently we get module name "something" k6 will try to load it as https:// + something in case it is a URL without a schema, which used to be the only way to load urls more than 4 years ago.

This is mostly confusing users when they make mistake with importing something instead of actually helping.

History

This functionality dates back to before my involvement with the project and in one of my first big rewrite #1046 it was left as backwards compatibility. Especially as that PR also let users actually prepend https:// infront of modules specifiers which made them more consistent and obvious.

Solution

Given that it has been 4 years - I am for dropping this functionality. I very much doubt anyone still uses that, and if they do they have been seeing the message all of this time.

We can also change the message to us actually deprecating it one release and removing it a few releases later, to be more consistent and to not break someone script. Although again I really doubt anyone has been using this with this message constantly logging.

@mstoykov mstoykov added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Aug 18, 2023
@mstoykov mstoykov added this to the v0.47.0 milestone Aug 18, 2023
@olegbespalov
Copy link
Collaborator

Let's go with removing that gueesing 👍

Alrougth having a release with the deprecation warning sounds like a good idea since at least we have tried not to "surprise" our customers.

@oleiade
Copy link
Member

oleiade commented Aug 18, 2023

I agree with the observation, and I'm happy with the solution. This specific use case happens to me all the time, and I find it indeed confusing and frustrating 👍🏻

@codebien
Copy link
Collaborator

codebien commented Aug 21, 2023

Alrougth having a release with the deprecation warning sounds like a good idea since at least we have tried not to "surprise" our customers.

We should do something more aggressive here, in concrete that message is already a deprecation process. If they really are using it and they still need it for whatever reason then they can avoid upgrading. I'm for dropping in the upcoming release.

I don't see the benefits in doing the deprecation process. In the end, we are not removing any functionality, and I'm not even sure that we should consider it as a breaking change.

@olegbespalov olegbespalov modified the milestones: v0.47.0, v0.48.0 Sep 20, 2023
mstoykov added a commit that referenced this issue Nov 9, 2023
This has been deprecated for years and never really made much sense, but
made a bunch of the error messages way more convoluted.

Closes #3287

This also fixes tc39 tests and some archive ones that were depending on
this functionality.

AFAIK all of those tests should've not been written the way they were to
begin with - they just happened to work due to the previous behaviour.
mstoykov added a commit that referenced this issue Nov 10, 2023
This has been deprecated for years and never really made much sense, but
made a bunch of the error messages way more convoluted.

Closes #3287

This also fixes tc39 tests and some archive ones that were depending on
this functionality.

AFAIK all of those tests should've not been written the way they were to
begin with - they just happened to work due to the previous behaviour.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes
Projects
None yet
4 participants