breaking change: Don't try to prepend https to module specifiers #3451
Conversation
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.
| func (n noSchemeRemoteModuleResolutionError) Unwrap() error { | ||
| return n.err | ||
| func (u unresolvableURLError) Error() string { | ||
| // TODO potentially add more things about what k6 supports if users report being confused. |
There was a problem hiding this comment.
It's probably a good idea to provide the list of the supported moduleSpecifiers to give a user an understanding of how they look like
There was a problem hiding this comment.
I really don't like our messages being super huge. Which is why I prefer to err on the side of them being short then giving a lot of information that potentially won't be useful.
In this case explaining the modules can be absolute file paths, with or with file://, https:// urls, or relative paths (and what they will be relative to) + the k6/* stuff is way too much text. That arguably won't even help you enough.
I am somewhat okay with linking to the docs https://grafana.com/docs/k6/latest/using-k6/modules/, but even that seems a bit longish.
Especially as my expectation is that the most common case will be buffer or something similar from nodejs or npm.
There was a problem hiding this comment.
Are we discussing this message?
The moduleSpecifier \"example.com/html\" couldn't be recognised as something k6 supports
How about something like 🤔
The module specifier \"example.com/html\" is unsupported. Supported module modifiers are `file://`, `https://`
There was a problem hiding this comment.
Or even less, gaining from the context:
The module specifier \"example.com/html\" is unsupported. Supported are file:// and https://.
There was a problem hiding this comment.
Are we discussing this message?
Isn't this what you wanted? Or did I misunderstood?
There was a problem hiding this comment.
I was trying to address this:
I really don't like our messages being super huge. Which is why I prefer to err on the side of them being short then giving a lot of information that potentially won't be useful.
I mean, the message that we return right now (got it from tests):
The moduleSpecifier \"example.com/html\" couldn't be recognised as something k6 supports
And suggesting that we consider modifying it to something that contains examples:
The module specifier \"example.com/html\" is unsupported. Supported are file:// and https://.
Anyway, it was a non-blocking suggestion, so feel free to ignore it and merge as it is 👍
There was a problem hiding this comment.
This does not mention both relative module specifier and neither does k6/net/grpc for example
There was a problem hiding this comment.
I've merged it for now - I would prefer to get some user feedback on this before doing something.
If you (or anyone else) disagree - please open an issue.
We can change the message fairly easy - even though it will need to be checked around in tests
olegbespalov
added
the
breaking change
What?
Stop trying to prepend
https://to module specifiers that couldn't be parsed in other ways.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.
Why?
This has been deprecated for years and never really made much sense, but made a bunch of the error messages way more convoluted.
Checklist
make lint) and all checks pass.make tests) and all tests pass.Related PR(s)/Issue(s)
Closes #3287