loader-returned sources always-cached when imported with assert { type: 'json' }

[iambumblehead]

Version

v20.6.1

Platform

Linux duck 6.5.2-arch1-1 #1 SMP PREEMPT_DYNAMIC Wed, 06 Sep 2023 21:01:01 +0000 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

1. clone this reproduction repo https://github.com/iambumblehead/nodejs-import-attributes-repro
2. cd into the cloned directory and use npm test

How often does it reproduce? Is there a required condition?

the issue is reproduced every time

What is the expected behavior? Why is that the expected behavior?

when json is imported with "assert" this way,

import JSONobj from './example.json' assert { type: 'json' }

it should be possible for the loader to return dynamic, un-cached values to the importing file, following current behaviour around normally imported modules eg import JSobj from './example.js'

What do you see instead?

Instead, when "assert" is used to import a json file, cached results are returned to the importing file, irregardless of what source value is returned by the loader. The example test attached below fails, but should pass.

---

import test from 'node:test'
import assert from 'node:assert'
import module from 'node:module'

module.register('./loader.js', {
  parentURL: import.meta.url
})

// _importing.js_
// ```
// import JSobj from './example.js'
// import JSONobj from './example.json' assert { type: 'json' }
// 
// export { JSobj, JSONobj }
// ```

// for "module" type modules, the loader can return different versions
// of that "module" source to multiple import contexts (desired behaviour),
test('loader can return fresh source, js', async () => {
  const importOne = await import('./importing.js?t=1')
  const importTwo = await import('./importing.js?t=2')

  assert.deepEqual(importOne.JSobj, { example: 'js-1' })
  assert.deepEqual(importTwo.JSobj, { example: 'js-2' })
})

// for "json" type modules, the loader cannot return different versions
// of that "json" source to different import contexts and, instead, the
// first version is seemingly cache-returned to any subsequently-importing 
// context (not-desired behaviour)
test('loader cannot return fresh source, json', async () => {
  const importThree = await import('./importing.js?t=3')
  const importFour = await import('./importing.js?t=4')

  assert.deepEqual(importThree.JSONobj, { example: 'json-3' })
  assert.deepEqual(importFour.JSONobj, { example: 'json-4' })
  // ^^ fails: importFour.JSONobj == `{ example: 'json-3' }`
})

Additional information

related link about import attributes https://github.com/tc39/proposal-import-attributes#history

2023-03: The proposal is renamed to Import attributes and moves back to Stage 3 (TODO: notes, slides). The restriction on the cache key is completely removed, and the keyword changes back from assert to with: import { x } from "./mod" with { type: "json" };. For compatibility with existing implementations, the assert keyword will still be supported until it's safe to remove it, if it will ever be.

[aduh95]

Currently, you can only have "one absolute URL <=> one module"; it's a limitation we inherit from the HTML spec IIRC. You need to make changes in your resolve loader so it produces a different URL every time you want a different module to be loaded – otherwise it will return the one that's already on the LoadCache.

[iambumblehead]

You need to make changes in your resolve loader so it produces a different URL every time you want a different module to be loaded

The loader is doing this: resolved.url = resolved.url + match link

The json file alone is always returned from LoadCache regardless of url or query params

The reproduction repo seeks to demonstrate 1) the url is modified by the loader 2) json returned to the main thread is cached. What other corrections to the demo loader would return un-cached json to the unit-test?

My own numerous attempts mutating the url at the loader failed to break the cache

[aduh95]

I created a minimal repro:

import { register } from 'node:module';
import assert from 'node:assert';

async function resolve(referrer, context, next) {
  const result = await next(referrer, context);
  const url = new URL(result.url)
  url.searchParams.set('randomSeed', Math.random());
  result.url = url.href;
  return result;
}

function load(url, context, next) {
  if (context.importAssertions.type === 'json') {
    return {
      shortCircuit: true,
      format: 'json',
      source: JSON.stringify({ data: Math.random() }),
     };
  }
  return next(url, context);
}

register(`data:text/javascript,export ${encodeURIComponent(resolve)};export ${encodeURIComponent(load)}`);

assert.notDeepStrictEqual(
  await import('./file.json', { assert: { type: 'json' } }),
  await import('./file.json', { assert: { type: 'json' } }),
);

/cc @nodejs/loaders

[GeoffreyBooth]

Is there a minimal repro that doesn’t involve customization hooks? In particular, I want some code that can be run either in Node or in browsers, to see how browsers behave (so that we can copy their behavior).

[iambumblehead]

Respectfully, the issue reported here is a "loader issue" and reproducing the issue requires a loader. Browsers do not provide loader hooks to reproduce this issue, how can they be related to this issue?

[GeoffreyBooth]

Respectfully, the issue reported here is a “loader issue” and reproducing the issue requires a loader.

Sorry, I thought it was a bug regardless and loaders were just providing the way to reproduce it. So you’re saying that the default, no hooks registered scenario is fine and matches browsers? It’s only when hooks are registered that the issue appears, and it’s unrelated to the content of the hooks themselves (as in, the hooks in question aren’t caching when they shouldn’t)?

[isaacs]

I think this is actually a bug regardless, even when loaders are not used.

Minimal repro:

import { writeFileSync } from 'fs'
import assert from 'assert'
import test from 'node:test'

test('json', async () => {
  writeFileSync('foo.json', JSON.stringify({ firstJson: true }))
  const firstJson = await import('./foo.json?a=1', {
    assert: { type: 'json' },
  })
  writeFileSync('foo.json', JSON.stringify({ firstJson: false }))
  const secondJson = await import('./foo.json?a=2', {
    assert: { type: 'json' },
  })

  assert.notDeepEqual(firstJson, secondJson)
})

test('js', async () => {
  writeFileSync('foo.mjs', `
  export const firstJS = true
  `)
  const firstJS = await import('./foo.mjs?a=1')
  writeFileSync('foo.mjs', `
  export const firstJS = false
  `)
  const secondJS = await import('./foo.mjs?a=2')
  assert.notDeepEqual(firstJS, secondJS)
})

It seems to be caching based on the filename, not the import url. When loading JavaScript (or anything that doesn't use assert, I guess?) it caches based on the URL, so the js test passes.

[iambumblehead]

So you’re saying that the default, no hooks registered scenario is fine and matches browsers?

Yes, the default, no hooks registered scenario is fine and matches browsers afaik.

When a hook returns a "json" modified source it is not returned to the importing module and instead a cached source is returned.

This is different from "module" and "commonjs" sources. When a hook returns "module" or "commonjs" modified sources, those modified sources are returned to the importing module.

[aduh95]

Yes, the default, no hooks registered scenario is fine and matches browsers afaik.

No it doesn't:

/// index.mjs
const assert = {type:'json'};
const importWithoutQuery = await import("./package.json", {assert});
const importWithQuery = await import("./package.json?key=value", {assert});
console.log(
  JSON.stringify(importWithQuery.default) !== JSON.stringify(importWithoutQuery.default)
);

/// server.js
import http from 'node:http';
import {createReadStream} from 'node:fs';

http.createServer((req, res) => {
  switch(req.url) {
    case '/':
      res.setHeader('Content-Type', 'text/html');
      res.end('<script type=module>import "/index.mjs"</script>');
      return;
    case '/index.mjs':
      res.setHeader('Content-Type', 'text/javascript');
      createReadStream(new URL('./index.mjs', import.meta.url)).pipe(res);
      return;
    default:
        res.setHeader('Content-Type', 'application/json');
        res.end(JSON.stringify({ data: Math.random() }))
        return;
  }
}).listen(8080);

/// node.mjs
import { register } from 'node:module';

function load(url, context, next) {
  if (context.importAssertions.type === 'json') {
    return {
      shortCircuit: true,
      format: 'json',
      source: JSON.stringify({ data: Math.random() }),
     };
  }
  return next(url, context);
}

register(`data:text/javascript,export ${encodeURIComponent(load)}`);

/// package.json
{}

Node.js will print false when browsers (Safari and Chromium, and node --experimental-network-imports) print true.

$ node --import ./node.mjs index.mjs
(node:66937) ExperimentalWarning: Importing JSON modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
false
$ node server.mjs &
$ node --experimental-network-imports --input-type=module -e 'import "http://localhost:8080/index.mjs"'
(node:67034) ExperimentalWarning: Network Imports is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
(node:67034) ExperimentalWarning: Import assertions are not a stable feature of the JavaScript language. Avoid relying on their current behavior and syntax as those might change in a future version of Node.js.
(node:67034) ExperimentalWarning: Importing JSON modules is an experimental feature and might change at any time
true

The reason is that the Node.js ESM loader, when loading a JSON file from a file: URL, checks the CJS cache first; the reason for this choice was that in case CJS and ESM code try to load the same JSON file, they get the same data. Maybe we should restrict that behavior to when there's no query string in the loaded URL.

[isaacs]

Maybe we should restrict that behavior to when there's no query string in the loaded URL.

(Or hash.)

I think that would be the most expected outcome. ./package.json and ./package.json?x#y are completely different things from a require() point of view. They ought to be from an import point of view as well (and indeed, they are, except in this instance).

Another (more complicated, probably worse, maybe more performant) approach would be to only look to the require cache if the file is loaded from disk, and cache the stat (which I believe it does already), and only return the cached value if the mtime, dev, and ino all match what it saw the last time it loaded. Would save a file read and prevent the surprise, but the reason I'm guessing it's "probably worse" is that it might be quite a bit more complexity for not much performance benefit.

[iambumblehead]

No it doesn't

Sorry everyone, I submitted my comment just after @isaacs' comment and probably should have deleted or edited that. At that time, I wasn't aware of the general issue and believed the problem was loader-specific. I agree with things @aduh95 and @isaacs are messaging and have no additional input now.