Emit warning if require will not comply with specification

[mstoykov]

What?

Add warning on require invocations that do not follow the specification.

Depends on #3680

Why?

As this isn't how it works we would like to fix it.

Also happens to be hard to keep it working once we move to ESM.

part of #3534

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)

[codebien]

@mstoykov do you intend to open the pull request dropping the support directly now? It would be useful to be sure we are not going to forget to act on this.

[mstoykov]

Finally managed to finish it in #3694

[joanlopez]

Is this always safe? Is there any change to get frames with only one or no items at all?🤔

[mstoykov]

In all of the experiments I have ran in the last 4 months or so (this particular code is very old) I have had 1 panic when I called it when there was nothing running on the runtime.

As I would argue that is very much the wrong way to use it ... I am particularly certain it makes sense to capture this it seems.

The base branch was changed.

[codecov-commenter]

Codecov Report

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

Project coverage is 73.28%. Comparing base (edd2d15) to head (85d4b1e).

❗ Current head 85d4b1e differs from pull request most recent head 79d8dab. Consider uploading reports for the commit 79d8dab to get more accurate results

Files	Patch %	Lines
js/modules/require_impl.go	56.86%	16 Missing and 6 partials ⚠️
js/modules/resolution.go	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3681      +/-   ##
==========================================
+ Coverage   73.26%   73.28%   +0.02%     
==========================================
  Files         276      278       +2     
  Lines       20333    20386      +53     
==========================================
+ Hits        14897    14940      +43     
- Misses       4486     4496      +10     
  Partials      950      950              

Flag	Coverage Δ
macos	73.19% <57.89%> (?)
ubuntu	73.22% <57.89%> (-0.05%)	⬇️
windows	73.12% <57.89%> (?)

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.

[olegbespalov]

Generally, it looks good to me.

One thing that could be improved is to migrate from the terms "correct" and "wrong" words.

Maybe instead, the wrong use of CommonJS specification is non-compliant? I know that's longer, but I'm trying to think from the user perspective how they understand what's the issue faster. WDYT?

[mstoykov]

@olegbespalov I used to call it "compliant" earlier .. but then I had to explain to what it is "compliant" and it becomes longer and longer.

Arguably the linked issue is the much better place to find why this is happening and what needs to be done.

Also as I explain in the issue - you are really unlikely to hit this issue at all. If you just use ESM syntax - babel transpile it correctly. If not ... you literally need to be doing dynamic import require which AFAIK nobody is doing.

So I expect this will have practically 0 comments. But we will see 🤞