Closed Bug 1789329 Opened 2 years ago Closed 2 years ago

Sticky highlighting and context menu don't work for .sys.mjs files because searchfox doesn't know how to parse JS files as modules

Categories

(Webtools :: Searchfox, defect)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aminomancer, Assigned: asuth)

References

Details

Attachments

(3 files)

Lots of JSM modules have been migrated to ESM and had their file extensions changed to .sys.mjs recently. And when viewing these files on Searchfox (such as UrlbarInput.sys.mjs for example), we're unable to use the context menu and sticky highlighting as we normally could for JS files.

Regular syntax highlighting is working, just not sticky highlighting, and the only context menu item that appears is the "search for substring" one. If you look at the page DOM you can see the ANALYSIS_DATA and SYM_INFO dictionaries are empty.

It works fine for files ending in just .mjs (see featureCallout.mjs for example). I don't know anything about rust but I get the sense from the docs that the extra .sys shouldn't cause a problem.

The analysis pass says:

ERROR Unable to parse JS file /mnt/index-scratch/mozilla-central/git/browser/components/urlbar/UrlbarInput.sys.mjs:1.

ERROR because SyntaxError: import declarations may only appear at top level of a module: /mnt/index-scratch/mozilla-central/git/browser/components/urlbar/UrlbarInput.sys.mjs:7

Presumably the call to Reflect.parse needs to indicate when it wants to parse in a module context and when it does not. Since we've adopted the "mjs" suffix, the analyzer could use this as a heuristic, but I've also proposed in bug 1775130 that in general it would be nice if we could leverage our eslint jobs' understanding of things to do a better job here as it wouldn't surprise me if we have tests that do not use the extension, etc. Bug 1740290 also proposes trying to use scip-typescript for analysis instead of existing hand-rolled analysis job which could leverage {js,ts}config.json for meta-info, etc.

Although it appears MDN has removed docs about Reflect.parse because of its non-standard nature (there's nothing at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect) and nothing has showed up on firefox-source-docs (although there is mention of it in a checklist on https://firefox-source-docs.mozilla.org/js/feature_checklist.html) the forked (older) copy of MDN at https://udn.realityripple.com/docs/Mozilla/Projects/SpiderMonkey/Parser_API does indicate that we should be specifying a target of module which is currently defaulting to script (which we've left unspecified).

Summary: Sticky highlighting and context menu don't work for .sys.mjs files → Sticky highlighting and context menu don't work for .sys.mjs files because searchfox doesn't know how to parse JS files as modules

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #1)

Since we've adopted the "mjs" suffix, the analyzer could use this as a heuristic, but I've also proposed in bug 1775130 that in general it would be nice if we could leverage our eslint jobs' understanding of things to do a better job here as it wouldn't surprise me if we have tests that do not use the extension, etc.

ESlint has a little bit of sourceType information, but not for .mjs files - they are assumed to be modules per standard.

In bug 1773467, I'm also trying to aim for every module to have a .mjs extension so that we don't need ESLint's sourceType definitions. AFAIK there's no practical reason why this wouldn't work.

That's great news! Thanks for the extra context and your ongoing work to normalize these things so things are more straightforward for tooling (and humans ;)!

:gijs provided a fix on https://github.com/mozsearch/mozsearch/pull/568 (Thank you!). Validating this with a "dev" channel run right now that will show up on https://dev.searchfox.org/ in about 1.5 hours.

I'm going to file a follow-up bug to add JS test coverage for the in-tree "tests" repo and potentially for mozilla-central for production if we can find or place some files that aren't likely to bit-rot. Our JS fidelity is currently going to be quite good from the tests repo, so that might be good enough, although longer term maybe we could have crossref compile some metrics that could be used to trip a circuit breaker. Like if we suddenly no longer have any non-test JS definitions under browser/, that probably means things are broken.

As a meta thing, we should probably also be thinking about getting some kind of Continuous Integration setup going. We had a travis CI setup at some point but I don't know that it did anything more than validate provisioning.

Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Blocks: 1790502

Unfortunately, the dev run revealed that there's other module related stuff that we don't support, which isn't totally surprising, although I had thought we had made the JS analyzer not care so much about JS problems, but I guess it does! The specific error that broke the indexing run (on `browser/actors/AboutPluginsParent.sys.mjs):

/home/ubuntu/mozsearch/scripts/js-analyze.js:326:12 uncaught exception: Unexpected statement: ExportDeclaration

I reverted the fix. We'll want to add a representative module to the "tests" corpus with a check.

Assignee: gijskruitbosch+bugs → nobody
Status: ASSIGNED → NEW
Assignee: nobody → bugmail
Status: NEW → ASSIGNED

This is now fixed; https://searchfox.org/mozilla-central/raw-analysis/browser/components/urlbar/UrlbarInput.sys.mjs has extensive analysis data and we see properly sticky nesting when viewing the rendered source tip at https://searchfox.org/mozilla-central/source/browser/components/urlbar/UrlbarInput.sys.mjs. Bug 1792322 adds a quality ratchet so that warnings will be reported on the searchfox mailing list that reports errors and warnings (which I'm happy to add people to! :), and hopefully with the last landing and run happening right now, we'll be starting from 0 warnings so that we can actually pay attention to it.

For posterity, there is some interesting discussion about getting searchfox working under windows at https://github.com/mozsearch/mozsearch/pull/569 and while I still have a bit more work to do in bug 1612525 in terms of documentation[1] and helping script setup a little bit under WSL2, the docker experience is quite usable via ./build-docker.sh and ./run-docker.sh.

1: It's very frustrating that copying stuff out of a google doc into an editor like VS code tends to lose bullet-point formatting...

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: