ES module testing and import maps

wraiford commented 1 year ago

tl;dr

Is there an existing mechanism that I'm not seeing for import maps specifically? Or is there a more generic way to inject code into the run.html.ejs file?

context

I'm looking to build multiple packages and exploring options to try to minimize complexity. Currently I have jasmine working with testing ES modules in both node and in the browser in my base package with no imports with bare references. But as soon as I try to consume my base package in another package and test, I am unable to do so in the browser context (it works in node jasmine) since the browser does not know how to resolve the reference.

import { clone } from 'ts-gib';

succeeds in node jasmine but produces an error in the browser:

Uncaught TypeError: Failed to resolve module specifier "ts-gib". Relative references must start with either "/", "./", or "../".

workaround

If I manually modify my node_modules/jasmine-browser-runner/run.html.ejs to include an importmap script tag and then execute the jasmine-browser-runner (either runSpecs or serve), it works as expected.

<script type="importmap">
  {
    "imports": {
      "ts-gib": "https://unpkg.com/ts-gib@0.4.3/dist/browser/index.mjs"
    }
  }
</script>
...other script tags

Started . 1 spec, 0 failures

additional comments

As an aside, with so much movement towards ES modules (even TypeScript v5 will be using them internally), I think this could be a huge opportunity going forward for whoever makes testing in both node and browsers with ES modules the easiest. The landscape is ever changing of course, but at the very least import maps seem to have been standardized in most browsers.

sgravrock commented 1 year ago

That seems like a useful feature. I think it'd be better to let users specifically configure an import map than to provide a general way to inject code into the template. Partly because I think it'll make a useful capability more obvious, and partly because it doesn't create the risk of us breaking user configurations if we change the HTML in the template. Want to submit a PR?

wraiford commented 1 year ago

Eesh, I've been meaning to "help" another online project, especially one that I rely on (I try to minimize deps for better or worse 😬 ). I'll see what I can come up with.

How would you think the consumer would provide the imports:

entry in the jasmine-browser.json config
html file
js/ts file

Since the import map in the script tag is itself a key/value, I would imagine (1) would be the most natural fit.

As for the actual injection pieces, do you have an idea where in the codebase you would do it? I've grokked the overall structure, but I do relatively little with raw JS and more have viewed it through the TS transpilation lens these past several years. Also there's always little nooks and crannies in mature code bases it seems. 🙄

sgravrock commented 1 year ago

How would you think the consumer would provide the imports:
1. entry in the jasmine-browser.json config
2. html file
3. js/ts file
Since the import map in the script tag is itself a key/value, I would imagine (1) would be the most natural fit.

Yes. That seems like it'd be both best for users and easiest to implement.

As for the actual injection pieces, do you have an idea where in the codebase you would do it? I've grokked the overall structure, but I do relatively little with raw JS and more have viewed it through the TS transpilation lens these past several years. Also there's always little nooks and crannies in mature code bases it seems. 🙄

You shouldn't really need to wire anything up, because the configuration is already passed to the Server constructor more or less unmodified. I think these are the parts of the code that would need modification:

run.html.ejs to actually output the import map.
server.js to provide the import map to run.html.ejs.
JSDocs for Configuration in types.js.
Tests for the new functionality.

wraiford commented 1 year ago

I've done a small commit for just the validation of the config (and specs) (although rereading your msg, I see the config.js wasn't there! 😬 ). Could be too heavy for you in terms of validation, I just am in the camp of it's hard to have too much.

I included notes in the git commit message, but basically I just want to see your opinions on the overall style and such. I know you're busy, so I'll go ahead and start sometime this week on the other pieces. But since it's early days, it's better to get stylistic decisions top-down early so I can hopefully minimize those kinds of changes. I am naturally more verbose than most programmers, and I'll need help to try to speak your language better. I am mainly thinking of the consumer's POV and it helps knowing if you do a silly mistake like an empty string key or something (which doesn't make sense for an import map afaict).

wraiford commented 1 year ago

I've gone ahead and pushed what I have for the types.js in this commit.

I am not an expert with jsdoc (more proficient with TS), but I've tried to emulate existing code. I was unsure if jsdoc allowed nested object definitions:

/**
 * Map of one or more scoped module specifier maps.
 * @name ImportMappingsInfo#scopes
 * @type {Object.<string, Object.<string, string>>}
 */

note: I have since changed the ImportMapping to ImportMap in various places, as this seems to make more sense from the consumer's POV.

I'm still working on the import map ejs injection and testing.

sgravrock commented 1 year ago

That look pretty good for the most part, with a couple of things that I'd change. Responding to some of your questions first:

Could be too heavy for you in terms of validation, I just am in the camp of it's hard to have too much.

More validation seems better to me as long as it doesn't go too far and reject invalid import maps. What you have here seems fine, at a glance.

I was unsure if jsdoc allowed nested object definitions:

I tried out the example you gave and it seemed to work. In any case, if there are minor jsdoc syntax issues I can fix them post-merge.

note: I have since changed the ImportMapping to ImportMap in various places, as this seems to make more sense from the consumer's POV.

Agreed. Import Map seems to be the commonly used term, so going with that makes sense to me.

Some stylistic issue:

In Jasmine, the public API reference docs are autogenerated from JSDoc comments. Since validateImports and errorMsgHas aren't part of the public API, they shouldn't have JSDoc comments. Feel free to keep a comment that does not start with /** if you like, but in general I find that the JSDoc style formality is overkill for internal functions that only have one or two nearby call sites.
I think that it would be better to remove the const k = 'importMappings' variable in config.js. It adds indirection, which imposes a cost every time anyone reads the code in the future, and I'm not convinced that it's carrying its weight.
Similarly, the specs would be more readable if the kName variable was removed.

wraiford commented 1 year ago

Ah, thanks for the input. These are definitely the types of local choices to learn early on.

More validation seems better to me as long as it doesn't go too far and reject invalid import maps.

I'm assuming you mean reject valid import maps and I agree. I just have a habit of writing more tests than necessary while developing and feeling out wth I'm doing, but eventually it accumulates and can cost in overall test execution time. It's always easier to remove what we see as superfluous tests in the end. But I see this as your call. The point here is that there is an import map spec, and we can:

Commit to verifying the entire spec, including various and sundry like allowing for empty maps and ensuring trailing slashes match in both keys/values.
Give next-to-no validation other than defensive injection validation from our POV.
Give some easy low-hanging fruit with regards to verification of the consumer's import maps (like empty keys), so we can give them a little nudge with a helpful error message in case they forget something.

Obviously I went with no. 3, but this may give a false sense of security if the consumer thinks that it's a full verification.

Btw, WRT to no. 2 defensive injection validation, is there such a worry with this? Or more generally, besides your earlier point of not allowing arbitrary injection into the html template, is there another validation check I should be doing? I don't know all of the scenarios that jasmine is used or how this could be wielded, but I'd be glad to add in more regexp checks/whatever if needed. I don't think encoding would be good, but maybe something that disallows for anything to contain the string '<script>' sounds like it might be good. But like you said, I'd hate to have a valid mapping fail (annoyingly from the consumer's POV).

In Jasmine, the public API reference docs are autogenerated from JSDoc comments. Since validateImports and errorMsgHas aren't part of the public API, they shouldn't have JSDoc comments. Feel free to keep a comment that does not start with /** if you like, but in general I find that the JSDoc style formality is overkill for internal functions that only have one or two nearby call sites.

I am from the TS-vscode world and do it largely for my own auto-completion and a signature reference during dev, as opposed to document generation. Vscode also enables easy jsdoc generation WRT the /**/ style comments, but I am not trying to sway you on this or anything. Besides, I found that toThrowError has a regexp overload so I was able to simplify here. You can see the changes here, as well as other cleanup. Hey it's cool that I'm getting a better look into the api for my own usage too.

I think that it would be better to remove the const k = 'importMappings' variable in config.js. It adds indirection, which imposes a cost every time anyone reads the code in the future, and I'm not convinced that it's carrying its weight. Similarly, the specs would be more readable if the kName variable was removed.

Yes, it's hideous! You can say it I don't mind! (But I appreciate your being polite here for sure). I have that in there for the 2nd of 2 hard problems in CS. Early on it's nice to have the refactoring capability with interpolated strings like that and not just rely on a regex find/replace which can be scary in brownfield (from my POV) projects. The commit for this cleanup can be found here.

sgravrock commented 1 year ago

Btw, WRT to no. 2 defensive injection validation, is there such a worry with this? Or more generally, besides your earlier point of not allowing arbitrary injection into the html template, is there another validation check I should be doing? I don't know all of the scenarios that jasmine is used or how this could be wielded, but I'd be glad to add in more regexp checks/whatever if needed. I don't think encoding would be good, but maybe something that disallows for anything to contain the string ' Githubissues.
Githubissues is a development platform for aggregating issues.

jasmine / jasmine-browser-runner