brillout / react-streaming

React Streaming. Full-fledged & Easy.
MIT License
220 stars 14 forks source link

feat: add `default` in exports field #17

Closed bvanjoi closed 1 year ago

bvanjoi commented 1 year ago

According to https://nodejs.org/api/packages.html#conditional-exports, it should be better if there had a default field.

image

brillout commented 1 year ago

Is that webpack that wants a "default" export? Isn't it surprising that webpack doesn't consider the "browser" export?

bvanjoi commented 1 year ago

webpack adds browser to conditionNames by default when target = "web", which is fine when it brings in react-streaming, but bringing in react-streaming/server will report an error because conditionNames doesn't have worker in it.

Maybe you designed it that way on purpose?

brillout commented 1 year ago

I see. Thanks for the insights!

Do you know whether webpak has "server" in its conditions? Or, in general, what webpack's conditions are when it isn't targeting the browser?

brillout commented 1 year ago

I went ahead and removed the "default" condition for the main "." entry (I don't think it's needed). Only question left: can we use something more explicit than "default" for the "/server" entry? E.g. maybe even "webpack", if webpack has it – that'd be super explicit.

bvanjoi commented 1 year ago

Do you know whether webpak has "server" in its conditions?

I don't remember this happening

what webpack's conditions are when it isn't targeting the browser?

Note that this is the case without setting conditionNames:

bvanjoi commented 1 year ago

I went ahead and removed the "default" condition for the main "." entry (I don't think it's needed).

Yes, node and browser already cover almost all cases.

can we use something more explicit than "default" for the "/server" entry

This is up to you (my opinion is to use default), but I recommend not using webpack, because tools like vite may not have this default value?

brillout commented 1 year ago

Fix released at react-streaming@0.3.6.

You're right and I've drastically simplified the exports to this:

  "exports": {
    ".": {
      "node": "./dist/cjs/server/hooks.js",
      "default": "./dist/esm/client/hooks.js"
    },
    "./server": {
      "node": "./dist/cjs/server/index.js",
      "default": "./dist/esm/server/index.js"
    }
  }

Let's see if that one is going to work out. Hopefully so!

brillout commented 1 year ago

FYI ended up with:

  "exports": {
    ".": {
      "browser": "./dist/esm/client/hooks.js",
      "node": "./dist/cjs/server/hooks.js",
      "default": "./dist/esm/server/hooks.js"
    },
    "./server": {
      "node": "./dist/cjs/server/index.js",
      "default": "./dist/esm/server/index.js"
    }
  }