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

Fix printing wrong message on remote resolution #2530

Merged
merged 1 commit into from
May 26, 2022
Merged

Conversation

mstoykov
Copy link
Contributor

Previously we will always print the really long message that says we
tried everything. But in reality we only do this if we have something
that:

  • is not local (start with ./)
  • is not prefixed with https://
  • is not a magical github or cdnjs URL (which entirely uses the Opaque
    URL property)

But the if changed here basically checks that either we don't have
scheme or that it's Opaque. But in reality if it has scheme it never
has Opaque. And Opaque always has no scheme - so all cases enter the if.

The correct is to enter only when both are true as in that case it's
not magical and it doesn't have scheme so the schem

Previously we will always print the really long message that says we
tried everything. But in reality we only do this if we have something
that:
* is not local (start with ./)
* is not prefixed with `https://`
* is not a magical github or cdnjs URL (which entirely uses the Opaque
  URL property)

But the if changed here basically checks that either we don't have
scheme *or* that it's Opaque. But in reality if it has scheme it never
has Opaque. And Opaque always has no scheme - so all cases enter the if.

The correct is to enter *only* when both are true as in that case it's
not magical and it doesn't have scheme so the scheme was added by k6.
@mstoykov mstoykov added this to the v0.39.0 milestone May 12, 2022
@mstoykov mstoykov requested review from olegbespalov and oleiade May 18, 2022 13:12
@mstoykov mstoykov requested a review from codebien May 26, 2022 09:08
@codebien
Copy link
Contributor

codebien commented May 26, 2022

And Opaque always has no scheme - so all cases enter the if.

@mstoykov Apparently, the old error message mentions the nodejs case, where for example seems node:fs would be resolved with a scheme and opaque.

Example url resolution with Go:

u, err := url.Parse("node:fs")
fmt.Println(u.Scheme) // node
fmt.Println(u.Opaque) // fs

Is the logic before the if changing it making the scheme empty?

It seems the case is not covered by the tests, can we add it? It should help with "documenting" the case.

@mstoykov
Copy link
Contributor Author

mstoykov commented May 26, 2022

The message predates nodejs introducing the node scheme and is still printed in those cases ;) This actually exists for everybody who just wrote require("fs") or something similar

We don't really support anything but https and file as a scheme either way. But in this particular case it just sidestep this and gets in a totally different part of the code returning the same error.

Given that this still works I really don't want to touch this more as this code is convoluted enough. And until we decide to deprecate at least the https:// prefixing of everything it will continue to be so. If we decide to completely drop it (and especially if we also drop the special "magical" urls) I guess we will be able to in practice delete 80+% of the code that deals with those.

Even this fix in practice just makes a fairly rare case a bit less confusing, but if it required any more than this change I would probably just drop it.

@mstoykov mstoykov merged commit 9b51180 into master May 26, 2022
@mstoykov mstoykov deleted the fixLoaderMessage branch May 26, 2022 14:02
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.

3 participants