Deprecate magic resolving of cdnjs and github "URLs"

[mstoykov]

What?

Deprecate magic resolving of cdnjs and github "URLs"

Why?

As part of #1408 we are first going to deprecate it for a couple of versions and then remove it.

Checklist

• I have performed a self-review of my code.
• I have added tests for my changes.
• I have run linter locally (make lint) and all checks pass.
• I have run tests locally (make tests) and all tests pass.
• I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

#1408

[mstoykov]

Unfortunately adding a test in cmd/tests (where it seemed most relevant) hits the check that no goroutines are still alive at the end of the test. There are alive goroutines as the test actually hits the network and uses http2 to get the code. Unfortunately it also uses the default client and calling CloseIdleConnections on it doesn't do anything ;(.

Rewriting this so it can be more easily tested seems like quite a lot of work that also will mostly be thrown away once we have it removed so ... I decided not to go down this path.

Due to the way this is called internally the message is only printed once. It also doesn't get printed when running the archive, which might require additional code changes. But does get printed when creating the archive or running k6 cloud

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 73.54%. Comparing base (ba63df0) to head (0049e16).

❗ Current head 0049e16 differs from pull request most recent head 7b23ce9. Consider uploading reports for the commit 7b23ce9 to get more accurate results

Files	Patch %	Lines
loader/cdnjs.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3671      +/-   ##
==========================================
- Coverage   73.61%   73.54%   -0.08%     
==========================================
  Files         277      275       -2     
  Lines       20247    20246       -1     
==========================================
- Hits        14905    14890      -15     
- Misses       4396     4404       +8     
- Partials      946      952       +6     

Flag	Coverage Δ
macos	73.51% <42.85%> (-0.01%)	⬇️
ubuntu	73.54% <42.85%> (-0.01%)	⬇️
windows	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[codebien]

Suggested change

	const magicURLsDeprecationWarning = "Specifier %q resolved to use a non-conventional %s loader. " +
	"That loader is deprecated and will be removed in v0.53.0. Please use the real URL %q instead."
	const magicURLsDeprecationWarning = "Specifier %q was resolved by a non-conventional %s loader. " +
	"The used %q loader is deprecated and will be removed in k6 v0.53.0. Please, use the real URL %q instead."

In any case, my feeling is that non-conventional isn't clear enough information. Is there a de-facto list of conventional loaders for JavaScript that I'm not aware of?
I don't think we define somewhere the k6 conventional (supported?) loaders. We should explicit them here or in the documentation.

[mstoykov]

There isn't - and there is no documentation about cdnjs and github loaders for years at this point - we did drop it from every documentation like 3+ years ago.

I do agree - but IMO us making this message be more specific likely will have no benefit. I have not seen anyone use the github ones ever and the cdnjs ones seems to be used very rarely.

So my expectation is most people will never see this message.

If you have a concrete simple fix - I am all for it, but my only other proposal was "magic" (with quotes) and I think that is even less obvious.

I would also argue that most people will agree that pure paths and URLs are the "conventional" specifiers even if the specification has left this completely unspecified 😉 😉

[mstoykov]

I did redo practically the whole code so:

1. some parts of @codebien message changes were incorporated - the remaining ones were already kind of reverted from by @olegbespalov so I would prefer to not just flip-flop
2. This now works on running an archive, but doesn't print the real URL as in the case of cdnjs that will require making http requests, which goes against the whole idea of running from an archive. I don't really think this is such a big problem, but we can extend the message I guess.
3. The whole code changes is way more concentrated in one place.

I do think it migth be better to also link the original issue in the message, maybe as a way to also expand on the "non-conventional loader" term?

[codebien]

I do think it migth be better to also link the original issue in the message, maybe as a way to also expand on the "non-conventional loader" term?

As of now, the loader name is returned as part of the message, I find the current version enough. As we defined it as a rare case, I don't think we need more. What is wrong and what should be done to fix it should be clear to the user.

In any case, if you want to add it, I'm not against it.