Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exports field and worker bundles #435

Merged
merged 56 commits into from Jul 11, 2022
Merged

Conversation

nicksrandall
Copy link
Contributor

@nicksrandall nicksrandall commented Dec 9, 2021

See #431 for discussion.

I know the discussion hasn't finished on that issue but I figured while I was motivated, I would get the ball rolling with an implementation.

With these changes, users can now define "worker" bundles in the exact same way that they define "browser" bundles. The only difference between the two is that the worker bundle assumes typeof document, typeof window, and typeof process are all "undefined"

This PR now adds support to package exports field including "browser" and "worker" custom conditions. These changes should be mostly backwards compatible with previous versions. I used the "vanilla-extract" repo as a template and I have confirmed that these changes are at least compatible with their current config.

Remaining items

  • - Discuss simplifying the exports to make development bundles opt-in (to shrink pacakge.json size)
  • - Discuss adding support for the "types" conditional
  • - Test with latest and future versions of typescript to make sure everything works.

@changeset-bot
Copy link

changeset-bot bot commented Dec 9, 2021

🦋 Changeset detected

Latest commit: e73af13

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@preconstruct/cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@nicksrandall
Copy link
Contributor Author

nicksrandall commented Dec 10, 2021

Update: I have used this version of preconstruct and made the appropriate changes to the Emotion monorepo and then built a super simple react + emotion site and deployed to Cloudflare workers and it works!

Checkout: https://cloudflareworkers.com/?_gl=1*18veksy*_ga*MTQwOTc5MTU2MC4xNjM5MDY3NzEx*_gid*NjQ4MjQ2NTQuMTYzOTE2NjEyOQ..#bf591afbfb8e26d872b0dd273b7047e4:about:blank

packages/cli/src/build/config.ts Outdated Show resolved Hide resolved
packages/cli/src/build/rollup.ts Outdated Show resolved Hide resolved
{
format: "cjs" as const,
entryFileNames: "[name].worker.cjs.js",
chunkFileNames: "dist/[name]-[hash].worker.cjs.js",
Copy link
Member

@Andarist Andarist Dec 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: is a CJS file even needed for a worker target? Can't we assume that a consumer is able to understand ESM?

Copy link
Contributor Author

@nicksrandall nicksrandall Dec 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same argument could be made for browser target so I added this to be consistent. IMO, this is more about bundler preference than the target's preference.

I'm happy to take it out though if you feel like it is not needed.

Copy link
Member

@Andarist Andarist Dec 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your sentiment - it's just that the browser field has been created in the past, where the tooling's state was different. I assume that for the newly introduced exports we can start from a clean slate - I don't feel super strong about it though, my bundlers should just never pick up CJS files for those and that's it. I just wonder if we can skip generating those for perf reasons and to spare some disk space etc.

Let's wait for @mitchellhamilton's opinion

packages/cli/src/entrypoint.ts Outdated Show resolved Hide resolved
packages/cli/src/entrypoint.ts Outdated Show resolved Hide resolved
packages/cli/src/entrypoint.ts Outdated Show resolved Hide resolved
site/src/pages/configuration.mdx Outdated Show resolved Hide resolved
packages/cli/src/utils.ts Outdated Show resolved Hide resolved
@Andarist
Copy link
Member

Andarist commented Dec 10, 2021

This is awesome - thank you very much for working on this. I've left some open-ended questions - if you are any opinions of your own regarding those topics, please share them. We'll also have to wait for @mitchellhamilton's opinion about these. The topic is very delicate and we might not be able to merge this overnight - but this PR is a great start for making this a reality.

@nicksrandall
Copy link
Contributor Author

nicksrandall commented Dec 14, 2021

I added support for "production" and "development" package exports conditionals. I also added the ./package.json and ./ as default conditions as I see them used in various packages.

I ran this on the @emotion/react package to see what it would generate and it looks like this:

{
  "name": "@emotion/react",
  "version": "11.7.0",
  "main": "dist/emotion-react.cjs.js",
  "module": "dist/emotion-react.esm.js",
  "browser": {
    "./dist/emotion-react.cjs.js": "./dist/emotion-react.browser.cjs.js",
    "./dist/emotion-react.esm.js": "./dist/emotion-react.browser.esm.js"
  },
  "exports": {
    "./package.json": "./package.json",
    ".": {
      "worker": {
        "production": {
          "module": "./dist/emotion-react.worker.esm.prod.js",
          "default": "./dist/emotion-react.worker.cjs.prod.js"
        },
        "development": {
          "module": "./dist/emotion-react.worker.esm.dev.js",
          "default": "./dist/emotion-react.worker.cjs.dev.js"
        },
        "module": "./dist/emotion-react.worker.esm.js",
        "default": "./dist/emotion-react.worker.cjs.js"
      },
      "browser": {
        "production": {
          "module": "./dist/emotion-react.browser.esm.prod.js",
          "default": "./dist/emotion-react.browser.cjs.prod.js"
        },
        "development": {
          "module": "./dist/emotion-react.browser.esm.dev.js",
          "default": "./dist/emotion-react.browser.cjs.dev.js"
        },
        "module": "./dist/emotion-react.browser.esm.js",
        "default": "./dist/emotion-react.browser.cjs.js"
      },
      "production": {
        "module": "./dist/emotion-react.esm.prod.js",
        "default": "./dist/emotion-react.cjs.prod.js"
      },
      "development": {
        "module": "./dist/emotion-react.esm.dev.js",
        "default": "./dist/emotion-react.cjs.dev.js"
      },
      "module": "./dist/emotion-react.esm.js",
      "default": "./dist/emotion-react.cjs.js"
    },
    "./jsx-runtime": {
      "worker": {
        "production": {
          "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.worker.esm.prod.js",
          "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.worker.cjs.prod.js"
        },
        "development": {
          "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.worker.esm.dev.js",
          "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.worker.cjs.dev.js"
        },
        "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.worker.esm.js",
        "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.worker.cjs.js"
      },
      "browser": {
        "production": {
          "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.browser.esm.prod.js",
          "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.browser.cjs.prod.js"
        },
        "development": {
          "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.browser.esm.dev.js",
          "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.browser.cjs.dev.js"
        },
        "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.browser.esm.js",
        "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.browser.cjs.js"
      },
      "production": {
        "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.esm.prod.js",
        "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.cjs.prod.js"
      },
      "development": {
        "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.esm.dev.js",
        "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.cjs.dev.js"
      },
      "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.esm.js",
      "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.cjs.js"
    },
    "./jsx-dev-runtime": {
      "worker": {
        "production": {
          "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.worker.esm.prod.js",
          "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.worker.cjs.prod.js"
        },
        "development": {
          "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.worker.esm.dev.js",
          "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.worker.cjs.dev.js"
        },
        "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.worker.esm.js",
        "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.worker.cjs.js"
      },
      "browser": {
        "production": {
          "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.browser.esm.prod.js",
          "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.browser.cjs.prod.js"
        },
        "development": {
          "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.browser.esm.dev.js",
          "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.browser.cjs.dev.js"
        },
        "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.browser.esm.js",
        "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.browser.cjs.js"
      },
      "production": {
        "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.esm.prod.js",
        "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.cjs.prod.js"
      },
      "development": {
        "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.esm.dev.js",
        "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.cjs.dev.js"
      },
      "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.esm.js",
      "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.cjs.js"
    },
    "./_isolated-hnrs": {
      "worker": {
        "production": {
          "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.worker.esm.prod.js",
          "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.worker.cjs.prod.js"
        },
        "development": {
          "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.worker.esm.dev.js",
          "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.worker.cjs.dev.js"
        },
        "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.worker.esm.js",
        "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.worker.cjs.js"
      },
      "browser": {
        "production": {
          "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.browser.esm.prod.js",
          "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.browser.cjs.prod.js"
        },
        "development": {
          "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.browser.esm.dev.js",
          "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.browser.cjs.dev.js"
        },
        "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.browser.esm.js",
        "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.browser.cjs.js"
      },
      "production": {
        "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.esm.prod.js",
        "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.cjs.prod.js"
      },
      "development": {
        "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.esm.dev.js",
        "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.cjs.dev.js"
      },
      "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.esm.js",
      "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.cjs.js"
    },
    "./": "./"
  },
  "types": "types/index.d.ts",
  "files": [
    "src",
    "dist",
    "jsx-runtime",
    "jsx-dev-runtime",
    "_isolated-hnrs",
    "types/*.d.ts",
    "macro.js",
    "macro.d.ts",
    "macro.js.flow"
  ],
  "sideEffects": true,
  "author": "mitchellhamilton <mitchell@mitchellhamilton.me>",
  "license": "MIT",
  "scripts": {
    "test:typescript": "dtslint types"
  },
  "dependencies": {
    "@babel/runtime": "^7.13.10",
    "@emotion/cache": "^11.6.0",
    "@emotion/serialize": "^1.0.2",
    "@emotion/sheet": "^1.1.0",
    "@emotion/utils": "^1.0.0",
    "@emotion/weak-memoize": "^0.2.5",
    "hoist-non-react-statics": "^3.3.1"
  },
  "peerDependencies": {
    "@babel/core": "^7.0.0",
    "react": ">=16.8.0"
  },
  "peerDependenciesMeta": {
    "@babel/core": {
      "optional": true
    },
    "@types/react": {
      "optional": true
    }
  },
  "devDependencies": {
    "@babel/core": "^7.13.10",
    "@emotion/css": "11.5.0",
    "@emotion/css-prettifier": "1.0.0",
    "@emotion/server": "11.4.0",
    "@emotion/styled": "11.6.0",
    "@types/react": "^16.9.11",
    "dtslint": "^0.3.0",
    "html-tag-names": "^1.1.2",
    "react": "^17.0.2",
    "svg-tag-names": "^1.1.1"
  },
  "repository": "https://github.com/emotion-js/emotion/tree/main/packages/react",
  "publishConfig": {
    "access": "public"
  },
  "umd:main": "dist/emotion-react.umd.min.js",
  "preconstruct": {
    "entrypoints": [
      "./index.js",
      "./jsx-runtime.js",
      "./jsx-dev-runtime.js",
      "./_isolated-hnrs.js"
    ],
    "umdName": "emotionReact"
  }
}

Draft PR to emotion repo can be view here: emotion-js/emotion#2589

@Andarist Andarist mentioned this pull request Dec 14, 2021
@Andarist
Copy link
Member

Andarist commented Dec 14, 2021

I think we could minimize the output a little bit, like illustrated on this example

{
  "exports": {
    ".": {
      "worker": {
-        "production": {
-          "module": "./dist/emotion-react.worker.esm.prod.js",
-          "default": "./dist/emotion-react.worker.cjs.prod.js"
-        },
        "development": {
          "module": "./dist/emotion-react.worker.esm.dev.js",
          "default": "./dist/emotion-react.worker.cjs.dev.js"
        },
-        "module": "./dist/emotion-react.worker.esm.js",
+        "module": "./dist/emotion-react.worker.esm.prod.js",
-        "default": "./dist/emotion-react.worker.cjs.js"
+        "default": "./dist/emotion-react.worker.cjs.prod.js"
      }
    }
  }
}

This output makes production bundles to be chosen by default (as sort of a "safe" option) while making development bundles opt-in. And there is that open question if we even need CJS bundles for browser/worker conditions.

One thing that absolutely has to be checked before publishing this is to check if this behaves correctly with the current TS version and with the upcoming TS versions that will support package.json#exports. I know that the latter is somewhat still a moving target as this has not been published yet but hopefully this part, that we are going to rely on, should be stable(-ish) by now. I think @mitchellhamilton has done some early experiments here so he could tell us how this behaves right now in the nightly builds of TS. I expect that we might need to add an extra "types" condition for each entrypoint.

As we, most likely, would like to use Emotion as a testbed for this - we might need the whole entrypoints extensions for the MVP, this stuff is mentioned here. Otherwise, imports like @emotion/react/macro wouldn't work. This goes out of the scope of this PR and if you don't want to implement this - I would totally understand this and we can add this ourselves after landing this one first.

"./_isolated-hnrs.js"

This actually shouldn't be available publicly - but such "private" entrypoints sound like a super niche thing. So this is probably not worth solving anytime soon anyway.

@nicksrandall
Copy link
Contributor Author

nicksrandall commented Dec 16, 2021

@Andarist I have made package exports support strictly "opt-in" using the API you describe in #432. This includes support for the "extra" option to add arbitrary items to the package exports field when generating them.

Here is what @emotion/react package.json looks like now with these changes:

{
  "name": "@emotion/react",
  "version": "11.7.0",
  "main": "dist/emotion-react.cjs.js",
  "module": "dist/emotion-react.esm.js",
  "browser": {
    "./dist/emotion-react.cjs.js": "./dist/emotion-react.browser.cjs.js",
    "./dist/emotion-react.esm.js": "./dist/emotion-react.browser.esm.js"
  },
  "exports": {
    "./package.json": "./package.json",
    ".": {
      "worker": {
        "production": {
          "module": "./dist/emotion-react.worker.esm.prod.js",
          "default": "./dist/emotion-react.worker.cjs.prod.js"
        },
        "development": {
          "module": "./dist/emotion-react.worker.esm.dev.js",
          "default": "./dist/emotion-react.worker.cjs.dev.js"
        },
        "module": "./dist/emotion-react.worker.esm.js",
        "default": "./dist/emotion-react.worker.cjs.js"
      },
      "browser": {
        "production": {
          "module": "./dist/emotion-react.browser.esm.prod.js",
          "default": "./dist/emotion-react.browser.cjs.prod.js"
        },
        "development": {
          "module": "./dist/emotion-react.browser.esm.dev.js",
          "default": "./dist/emotion-react.browser.cjs.dev.js"
        },
        "module": "./dist/emotion-react.browser.esm.js",
        "default": "./dist/emotion-react.browser.cjs.js"
      },
      "production": {
        "module": "./dist/emotion-react.esm.prod.js",
        "default": "./dist/emotion-react.cjs.prod.js"
      },
      "development": {
        "module": "./dist/emotion-react.esm.dev.js",
        "default": "./dist/emotion-react.cjs.dev.js"
      },
      "module": "./dist/emotion-react.esm.js",
      "default": "./dist/emotion-react.cjs.js"
    },
    "./jsx-runtime": {
      "worker": {
        "production": {
          "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.worker.esm.prod.js",
          "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.worker.cjs.prod.js"
        },
        "development": {
          "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.worker.esm.dev.js",
          "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.worker.cjs.dev.js"
        },
        "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.worker.esm.js",
        "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.worker.cjs.js"
      },
      "browser": {
        "production": {
          "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.browser.esm.prod.js",
          "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.browser.cjs.prod.js"
        },
        "development": {
          "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.browser.esm.dev.js",
          "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.browser.cjs.dev.js"
        },
        "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.browser.esm.js",
        "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.browser.cjs.js"
      },
      "production": {
        "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.esm.prod.js",
        "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.cjs.prod.js"
      },
      "development": {
        "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.esm.dev.js",
        "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.cjs.dev.js"
      },
      "module": "./jsx-runtime/dist/emotion-react-jsx-runtime.esm.js",
      "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.cjs.js"
    },
    "./jsx-dev-runtime": {
      "worker": {
        "production": {
          "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.worker.esm.prod.js",
          "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.worker.cjs.prod.js"
        },
        "development": {
          "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.worker.esm.dev.js",
          "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.worker.cjs.dev.js"
        },
        "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.worker.esm.js",
        "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.worker.cjs.js"
      },
      "browser": {
        "production": {
          "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.browser.esm.prod.js",
          "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.browser.cjs.prod.js"
        },
        "development": {
          "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.browser.esm.dev.js",
          "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.browser.cjs.dev.js"
        },
        "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.browser.esm.js",
        "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.browser.cjs.js"
      },
      "production": {
        "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.esm.prod.js",
        "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.cjs.prod.js"
      },
      "development": {
        "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.esm.dev.js",
        "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.cjs.dev.js"
      },
      "module": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.esm.js",
      "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.cjs.js"
    },
    "./_isolated-hnrs": {
      "worker": {
        "production": {
          "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.worker.esm.prod.js",
          "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.worker.cjs.prod.js"
        },
        "development": {
          "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.worker.esm.dev.js",
          "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.worker.cjs.dev.js"
        },
        "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.worker.esm.js",
        "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.worker.cjs.js"
      },
      "browser": {
        "production": {
          "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.browser.esm.prod.js",
          "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.browser.cjs.prod.js"
        },
        "development": {
          "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.browser.esm.dev.js",
          "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.browser.cjs.dev.js"
        },
        "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.browser.esm.js",
        "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.browser.cjs.js"
      },
      "production": {
        "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.esm.prod.js",
        "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.cjs.prod.js"
      },
      "development": {
        "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.esm.dev.js",
        "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.cjs.dev.js"
      },
      "module": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.esm.js",
      "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.cjs.js"
    },
    "./macro": {
      "types": "./macro.d.ts",
      "default": "./macro.js"
    }
  },
  "types": "types/index.d.ts",
  "files": [
    "src",
    "dist",
    "jsx-runtime",
    "jsx-dev-runtime",
    "_isolated-hnrs",
    "types/*.d.ts",
    "macro.js",
    "macro.d.ts",
    "macro.js.flow"
  ],
  "sideEffects": true,
  "author": "mitchellhamilton <mitchell@mitchellhamilton.me>",
  "license": "MIT",
  "scripts": {
    "test:typescript": "dtslint types"
  },
  "dependencies": {
    "@babel/runtime": "^7.13.10",
    "@emotion/cache": "^11.6.0",
    "@emotion/serialize": "^1.0.2",
    "@emotion/sheet": "^1.1.0",
    "@emotion/utils": "^1.0.0",
    "@emotion/weak-memoize": "^0.2.5",
    "hoist-non-react-statics": "^3.3.1"
  },
  "peerDependencies": {
    "@babel/core": "^7.0.0",
    "react": ">=16.8.0"
  },
  "peerDependenciesMeta": {
    "@babel/core": {
      "optional": true
    },
    "@types/react": {
      "optional": true
    }
  },
  "devDependencies": {
    "@babel/core": "^7.13.10",
    "@emotion/css": "11.5.0",
    "@emotion/css-prettifier": "1.0.0",
    "@emotion/server": "11.4.0",
    "@emotion/styled": "11.6.0",
    "@types/react": "^16.9.11",
    "dtslint": "^0.3.0",
    "html-tag-names": "^1.1.2",
    "react": "^17.0.2",
    "svg-tag-names": "^1.1.1"
  },
  "repository": "https://github.com/emotion-js/emotion/tree/main/packages/react",
  "publishConfig": {
    "access": "public"
  },
  "umd:main": "dist/emotion-react.umd.min.js",
  "preconstruct": {
    "exports": {
      "extra": {
        "./macro": {
          "types": "./macro.d.ts",
          "default": "./macro.js"
        }
      }
    },
    "entrypoints": [
      "./index.js",
      "./jsx-runtime.js",
      "./jsx-dev-runtime.js",
      "./_isolated-hnrs.js"
    ],
    "umdName": "emotionReact"
  }
}

@nicksrandall
Copy link
Contributor Author

nicksrandall commented Dec 16, 2021

@Andarist @mitchellhamilton I think we're really close but I could use some help testing these changes on the latest typescript. Do you have any ideas on how I could do that?

@nicksrandall
Copy link
Contributor Author

nicksrandall commented Dec 16, 2021

I think we could minimize the output a little bit, like illustrated on this example

Is there value from making this change besides making the package.json file smaller?

@Andarist
Copy link
Member

Andarist commented Dec 16, 2021

@Andarist @mitchellhamilton I think we're really close but I could use some help testing these changes on the latest typescript. Do you have any ideas on how I could do that?

I would install typescript@next and create a simple package manually in node_modules (for testing purposes). This should allow for testing different things quickly without the need to publish anything at all.

Is there value from making this change besides making the package.json file smaller?

A file imported like this from 'lib/entry' would never rely on process.env.NODE_ENV. People would always load files with this already being replaced for them. This is a good thing as process.env.NODE_ENV thing is a weird convention and it doesn't work in every situation - for instance when working with bare Rollup you actually have to configure it to replace this with @rollup/plugin-replace and not everyone does this.

@emmatown
Copy link
Member

emmatown commented Dec 17, 2021

dev/prod bundle stuff

I'm not so sure about defaulting to the production bundle. Also note that the dev bundle doesn't have process.env.NODE_ENV replaced so you'd still kinda need process.env.NODE_ENV replacement. If we shipped this in Emotion, I would imagine that we would very quickly get new label mismatch errors since while sure, webpack will default to using the development condition in dev, most frameworks that do SSR don't bundle dependencies on the server and since I don't believe there is a way to dynamically change the conditions that Node uses and I don't believe any of these tools spawn processes with at least the development condition, they'd be using the production version in development.

@codecov
Copy link

codecov bot commented Dec 17, 2021

Codecov Report

Merging #435 (e73af13) into main (4e04c59) will increase coverage by 1.89%.
The diff coverage is 98.48%.

@@            Coverage Diff             @@
##             main     #435      +/-   ##
==========================================
+ Coverage   90.21%   92.11%   +1.89%     
==========================================
  Files          32       32              
  Lines        1319     1433     +114     
  Branches      350      403      +53     
==========================================
+ Hits         1190     1320     +130     
+ Misses        124      108      -16     
  Partials        5        5              
Impacted Files Coverage Δ
packages/cli/src/entrypoint.ts 100.00% <ø> (ø)
packages/cli/src/messages.ts 83.33% <ø> (ø)
packages/cli/src/project.ts 79.62% <ø> (ø)
packages/cli/src/build/config.ts 80.88% <85.71%> (-0.37%) ⬇️
packages/cli/src/package.ts 95.07% <98.24%> (+1.88%) ⬆️
packages/cli/src/build/rollup.ts 90.76% <100.00%> (+0.29%) ⬆️
packages/cli/src/dev.ts 98.41% <100.00%> (+0.05%) ⬆️
packages/cli/src/utils.ts 98.88% <100.00%> (+0.35%) ⬆️
packages/cli/src/validate-package.ts 92.42% <100.00%> (+4.61%) ⬆️
packages/cli/src/validate.ts 91.46% <100.00%> (+1.85%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e04c59...e73af13. Read the comment docs.

@emmatown
Copy link
Member

emmatown commented Dec 17, 2021

Also, this definitely needs to be behind an experimental flag, we won't get this right immediately and we need to be able to iterate on it without doing majors constantly

@emmatown
Copy link
Member

emmatown commented Dec 17, 2021

(i'm not very concerned about the coverage thing, that just showed up because I allowed CI to run. tests for running validate, fix and build with an exports field would be good though)

packages/cli/src/build/config.ts Outdated Show resolved Hide resolved
site/src/pages/configuration.mdx Outdated Show resolved Hide resolved
@nicksrandall
Copy link
Contributor Author

nicksrandall commented Dec 17, 2021

Also, this definitely needs to be behind an experimental flag, we won't get this right immediately and we need to be able to iterate on it without doing majors constantly

I'm happy to do this but I'm but sure how this is typically done. Is noting that this feature is experimental in the docs enough? Do I need to rename the opt-in config name to EXPERIMENTAL_exports or something like that?

@emmatown
Copy link
Member

emmatown commented Dec 17, 2021

You can add another flag to the ___experimentalFlags_WILL_CHANGE_IN_PATCH option, see https://github.com/preconstruct/preconstruct/pull/383/files for an example

@emmatown emmatown marked this pull request as ready for review Jul 7, 2022
@emmatown emmatown requested a review from Andarist Jul 7, 2022
@@ -305,3 +305,50 @@ test("typescript with typeScriptProxyFileWithImportEqualsRequireAndExportEquals"
"
`);
});

test("exports field with worker condition", async () => {
let tmpPath = realFs.realpathSync.native(
Copy link
Member

@Andarist Andarist Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: why do we use realpathSync.native here? some tests in this file do this and some dont

Copy link
Member

@emmatown emmatown Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests that read symlinks use realpathSync.native because (at least from my vague reading of things) realpath and realpath.native do subtly different things on Windows(like realpath seems to just not resolve some links and realpath.native does?) and that matters when getting the path that a symlink resolves to.

".": {
"module": {
"worker": "./dist/package-exports.worker.esm.js",
"browser": "./dist/package-exports.browser.esm.js",
Copy link
Member

@Andarist Andarist Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I don't think it's ideal to rely on the "caller"/bundler being configured with a module condition. Rollup always includes the module condition but I'm pretty sure that webpack doesn't if you override resolve.conditionNames (I didn't test it though). Usually used condition names probably should be configured indirectly through target but it is not unlikely that somebody configures it explicitly. In such a case it is likely that with conditionNames: ['browser'] they wouldn't pick up our exports.module.browser.

It also seems that esbuild doesn't support module condition by default - I've looked into its code and I don't see it there. Its docs also doesn't mention it: https://esbuild.github.io/api/#how-conditions-work

Throwing new tools, like bun, into the mix - we can't assume that module is always supported.

Copy link
Member

@emmatown emmatown Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying we should change something here or just "this sucks"? Because I agree it sucks but I don't see a way to make it better except for going Node ESM only (I think Node ESM only is the only practical way to go in the end, even though it of course sucks for other reasons. I'm imagining that Preconstruct will have a mode for what it does now and a Node ESM only mode)

Copy link
Member

@Andarist Andarist Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying we should change something here or just "this sucks"? Because I agree it sucks but I don't see a way to make it better except for going Node ESM only

Hm, I was only commenting on the module condition used with env conditions here. I agree that for now, we shouldn't allow node loading our ESM files.

Are you afraid here that even when using a bundler we could risk introducing a dual-package hazard when the module condition is not present because they might bundle using "node strict rules" when the exports are present? And that's why you have chosen to only allow those env conditions in combination with the module condition?

Copy link
Member

@emmatown emmatown Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sort of problem, yes, basically "the semantics of what all this stuff does is not well defined so do the thing that seems the least likely to break"

Copy link
Member

@Andarist Andarist Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A slight problem here is that it will start loading CJS for browser targets without the module condition. But I guess the only offender here is esbuild right now. Perhaps we should open an issue there about this.

// shortest entrypoints first since shorter entrypoints
// are generally more commonly used
const comparison = a.length - b.length;
if (comparison !== 0) return comparison;
Copy link
Member

@Andarist Andarist Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neat

}
```

Additionally, you'll also need to enable the feature on each individual pacakge.
Copy link
Member

@Andarist Andarist Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Additionally, you'll also need to enable the feature on each individual pacakge.
Additionally, you'll also need to enable the feature on each individual package.

Kinda a bummer - but I guess it's better for bigger monorepos that may not want to add this field everywhere just yet.

It would be cool to have a way to enforce that every package specifies this config, without it I need to be on my toes when adding new packages (arguably I usually copy-paste this stuff but the risk is still there).

Copy link
Member

@emmatown emmatown Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was always planning to have project-level config for it since that's what you'll probably want to do but didn't add it to this PR, might just add that now


`Array<"browser" | "worker">`

Specifying the `envConditions` option adds additional environments that Preconstruct will generate bundles for. This option is currently aimed at generating bundles with `typeof SOME_ENV_SPECIFIC_GLOBAL` replaced with what it would be in that environment. It may be expanded to provide the ability to have Preconstruct resolve a different file or etc. depending on the environment in the future.
Copy link
Member

@Andarist Andarist Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be expanded to provide the ability to have Preconstruct resolve a different file or etc. depending on the environment in the future.

We could mention that this already should, sort of, be supported thanks to package.json#imports.

Copy link
Member

@emmatown emmatown Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't currently - we don't pass the conditions to the resolve plugin.

Copy link
Member

@Andarist Andarist Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason why we don't do this? 🤔 It seems like it would be a good addition

Copy link
Member

@emmatown emmatown Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just didn't want to figure out the exact semantics as part of this PR

@@ -102,11 +103,23 @@ function createEntrypoints(
);
}

export type ExportsConditions = {
module: string | { worker?: string; browser?: string; default: string };
default: string;
Copy link

@steve-taylor steve-taylor Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naïve question: Where do module and default come from?

My understanding, based on Node.js conditional exports, is that import (not module) is the condition that points to the ES module and require (not default) points to the CommonJS module. default is the fallback condition. module isn't a standard condition, so the fallback would always be used, therefore no ESM support at all.

Like I said, naïve question. I must be missing something.

https://nodejs.org/docs/latest-v16.x/api/packages.html#packages_conditional_exports

Glad to see this is being worked on, though. I've been adding these manually in my packages just to get Next.js + Styled Components + any third-party ESM-only package(s) working together.

Copy link
Member

@Andarist Andarist Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some limitations of the approach in this PR are mentioned here. Perhaps this requires some additional information to give more context behind this decision.

module condition has been "invented" by webpack to avoid dual-package hazard for packages defining it. Rollup also supports this condition. It allows requester to load the same file (usually in the ESM format) regardless of it being loaded through import or require. It assumes that a consuming tool knows how to handle those scenarios - that's why node doesn't define it anyhow. After all it can't load ESM file using require.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that Preconstruct's support for the exports field does not currently include generating ESM compatible with Node.js. While ESM builds are generated, they are targeting bundlers, not Node.js or browsers directly so they use the module condition, not the import condition.

Are the ESM bundles themselves not compatible with Node.js? And that's why you're not including the import condition? Or is this only saying that the import condition is out of scope for this PR?
What happens to the import and require conditions that I'm manually adding? Will they be clobbered? So many questions!

Copy link
Member

@emmatown emmatown Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the ESM bundles themselves not compatible with Node.js?

Essentially yes because Node.js ESM has different semantics to the semantics commonly adopted by bundlers

expect(stdout.toString("utf8")).toMatchInlineSnapshot(`
"blah.ts(3,29): error TS2307: Cannot find module 'pkg-a/not-exported' or its corresponding type declarations.
blah.ts(11,22): error TS2345: Argument of type '\\"index\\"' is not assignable to parameter of type '\\"other\\"'.
blah.ts(12,22): error TS2345: Argument of type '\\"something\\"' is not assignable to parameter of type '\\"other\\"'.
"
`);
Copy link
Member

@Andarist Andarist Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we don't need to do anything special to support types? 🤔 They will still be resolved without the types condition just fine?

Copy link
Member

@emmatown emmatown Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a natural consequence of how our d.ts files exist, TypeScript just goes to the default condition, finds the js file and then finds the d.ts file next to it.

Copy link
Member

@Andarist Andarist Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've suspected smth like that - thanks for confirming 👍

Copy link
Member

@Andarist Andarist left a comment

  1. we do not support right now any form of glob-based entrypoints, right? I don't see any tests using smth like this and the getRollupConfig doesn't seem to support this. I just want to make sure we don't need to add any support/tests for subpath patterns
  2. It would be lovely to discuss production/development support after we land this

@emmatown
Copy link
Member

emmatown commented Jul 7, 2022

we do not support right now any form of glob-based entrypoints, right? I don't see any tests using smth like this and the getRollupConfig doesn't seem to support this. I just want to make sure we don't need to add any support/tests for subpath patterns

They're supported as they already are in Preconstruct but subpath patterns aren't used, there will be an entry for each entrypoint

@Andarist
Copy link
Member

Andarist commented Jul 7, 2022

Ah, got it - so entrypoints are always expanded up front (at least right now).

emmatown and others added 2 commits Jul 7, 2022
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
@emmatown emmatown merged commit 014038b into preconstruct:main Jul 11, 2022
11 checks passed
@github-actions github-actions bot mentioned this pull request Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants