Import from unified instead of relative index.js

[remcohaszing]

Initial checklist

• [x] I read the support docs
• [x] I read the contributing guide
• [x] I agree to follow the code of conduct
• [x] I searched issues and couldn’t find anything (or linked relevant results below)
• [x] If applicable, I’ve added docs and tests

Description of changes

In test code, imports now import from unified instead of a relative path to index.js. This way package exports are also tested, both runtime and for type checking. This truly tests the public interface.

I’m not saying we should go over all packages to apply this immediately, but when working on a package, I think it’s a good idea to apply this pattern. This PR is mostly to share and discuss the idea.

[wooorm]

This way package exports are also tested, both runtime and for type checking. This truly tests the public interface.

I don’t think this is really done this way? I think something like this checks it better. Or at least, together. I’d propose:

  const mod = await import('unified')
  assert.deepEqual(
    Object.keys(mod).sort(),
    ['unified'],
    'should expose the public api'
  )

If there are other exports, we can have such blocks for those too.

---

Furthermore, doesn’t this actually checks the nested unified, not the code we’re working on here? At least, this used to be something, as far as I was aware, that was only supported if an exports field was available? 🤔 And most of our packages don’t have that yet (which I’m for, just that, it’s a breaking change for not much benefit)?

[remcohaszing]

Furthermore, doesn’t this actually checks the nested unified, not the code we’re working on here? At least, this used to be something, as far as I was aware, that was only supported if an exports field was available? thinking And most of our packages don’t have that yet (which I’m for, just that, it’s a breaking change for not much benefit)?

You’re right, I was under the impression we already used export maps. I think it’s nice to use them, because it marks internals as private. This prevents accidental use, I think also of internal types. But indeed, it’s a breaking change.

---

I don’t think this is really done this way? I think something like this checks it better. Or at least, together. I’d propose:

  const mod = await import('unified')
  assert.deepEqual(
    Object.keys(mod).sort(),
    ['unified'],
    'should expose the public api'
  )

If there are other exports, we can have such blocks for those too.

Given the above statement that we don’t use export maps, I think you mean this:

  const mod = await import('../index.js')
  assert.deepEqual(
    Object.keys(mod).sort(),
    ['unified'],
    'should expose the public api'
  )

I don’t really see the value of this assertion TBH. We already test against the main index.js file, which is currently as close to the public interface as we can get.

---

Another benefit of my proposal (for packages that do use export maps) is that we can test against multiple export conditions. This is already done in https://github.com/syntax-tree/hast-util-from-html-isomorphic/blob/main/package.json#L64-L66 for example.

node --test
node --test --conditions browser
node --test --conditions worker

[wooorm]

I don’t really see the value of this assertion TBH. We already test against the main index.js file, which is currently as close to the public interface as we can get.

We don’t test what’s part of the API. It is good to test what is part of the API. Your last phrase, “which is currently as close to the public interface as we can get.” answers it: we can get closer.

[github-actions[bot]]

Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/* labels.