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

chore: prefix main and module fields with ./ #1451

Merged
merged 10 commits into from Jan 3, 2022

Conversation

ilkkao
Copy link
Contributor

@ilkkao ilkkao commented Dec 31, 2021

Description

Esbuild currently ignores the browser field in the core utilies package, causing jsdom inclusion which leads to errors (esbuild doesn't show the call stack so the snippet is not that informative) :

$ ts-node ./esbuild.ts
DEV mode: false
✘ [ERROR] Could not resolve "path"

    node_modules/jsdom/lib/api.js:2:21:
      2 │ const path = require("path");
        ╵                      ~~~~~~

  The package "path" wasn't found on the file system but is built into node. Are you trying to
  bundle for node? You can use "platform: 'node'" to do that, which will remove this error.

The file names in the mapping need to match exactly. Therefore I used the ./ notation for the main and module entries. This is in line how the other package internal files are references in the package.json.

Checklist

Esbuild ignored the browser field, causing jsdom inclusion which
lead to errors.

The file names in the mapping need to match exactly. Therefore
using the ./ notation for the main and module entries. This is in line
how the other package internal files are references in the package.json.
@changeset-bot
Copy link

changeset-bot bot commented Dec 31, 2021

🦋 Changeset detected

Latest commit: b4b0156

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

This PR includes changesets to release 35 packages
Name Type
jest-remirror Patch
prosemirror-paste-rules Patch
prosemirror-resizable-view Patch
prosemirror-suggest Patch
prosemirror-trailing-node Patch
remirror Patch
@remirror/core Patch
@remirror/core-constants Patch
@remirror/core-helpers Patch
@remirror/core-types Patch
@remirror/core-utils Patch
@remirror/dev Patch
@remirror/dom Patch
@remirror/extension-annotation Patch
@remirror/extension-bidi Patch
@remirror/extension-blockquote Patch
@remirror/extension-bold Patch
@remirror/extension-callout Patch
@remirror/extension-code Patch
@remirror/extension-code-block Patch
@remirror/extension-codemirror5 Patch
@remirror/extension-codemirror6 Patch
@remirror/extension-collaboration Patch
@remirror/extension-columns Patch
@remirror/extension-diff Patch
@remirror/extension-doc Patch
@remirror/extension-drop-cursor Patch
@remirror/extension-embed Patch
@remirror/extension-emoji Patch
@remirror/extension-epic-mode Patch
@remirror/extension-events Patch
@remirror/extension-file Patch
@remirror/extension-font-family Patch
@remirror/extension-font-size Patch
@remirror/react-editors 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

@github-actions github-actions bot added the type: changeset 🦋 A change has been made to the changesets folder. label Dec 31, 2021
@ilkkao
Copy link
Contributor Author

ilkkao commented Dec 31, 2021

here is how esbuild interprets main, module, and browser fields in package.json https://esbuild.github.io/api/#main-fields

For browser field it links to the spec here: https://gist.github.com/defunctzombie/4339901/49493836fb873ddaa4b8a7aa0ef2352119f69211

There the examples show that a path that doesn't start with ./ refers to separate module. Looks like esbuild strictly follows the spec.

There are many other places that use dist... instead of ./dist in main and module fields. I guess those should be updated even not strictly speaking necessary.

@ocavue
Copy link
Member

ocavue commented Jan 3, 2022

Hi. Thanks for your first PR ❤️

In addition to the esbuild link above, I also checked the documentation for some other tools. It seems that all tools agree that it's a good practice to add ./ at the beginning of main and module fields, so let's do it.

Node.js docs https://nodejs.org/docs/latest-v16.x/api/packages.html#main-entry-point-export

Main entry point export#

To set the main entry point for a package, it is advisable to define both "exports" and "main" in the package’s package.json file:

{
  "main": "./main.js",
  "exports": "./main.js"
}

esbuild https://esbuild.github.io/api/#main-fields

For package authors: If you want to author a package that uses the browser field in combination with the module field to fill out all four entries in the full CommonJS-vs-ESM and browser-vs-node compatibility matrix, you want to use the expanded form of the browser field that is a map instead of just a string:

{
  "main": "./node-cjs.js",
  "module": "./node-esm.js",
  "browser": {
    "./node-cjs.js": "./browser-cjs.js",
    "./node-esm.js": "./browser-esm.js"
  }
}

Vite https://vitejs.dev/guide/build.html#library-mode

Recommended package.json for your lib:

{
  "name": "my-lib",
  "files": ["dist"],
  "main": "./dist/my-lib.umd.js",
  "module": "./dist/my-lib.es.js",
  "exports": {
    ".": {
      "import": "./dist/my-lib.es.js",
      "require": "./dist/my-lib.umd.js"
    }
  }
}

We have a script to generate these fields in package.json. I will edit this script and release a new version for all NPM packages.

@github-actions github-actions bot added the type: support 📚 A change focused on improving the architecture and structure of the support directory. label Jan 3, 2022
@github-actions github-actions bot added framework: dom 🌐 Any changes to packages which are exclusively used for DOM-only codebases. framework: react ⚛️ Any changes to packages which are exclusively used for react code bases. package: @remirror/cli 📟 A label for the remirror cli. package: jest-prosemirror 🎭 A change has occured in the jest-prosemirror package. package: jest-remirror 🃏 Label for the jest-remirror package package: multishift 🔽 Add this label when updating the `multishift` package. package: prosemirror-suggest 💡 A change has occurred in prosemirror-suggest. type: ui 💅 Changes made to user facing components and styles. labels Jan 3, 2022
@ocavue
Copy link
Member

ocavue commented Jan 3, 2022

Preconstruct doesn't like ./. I have to add a workaround for Preconstruct. This will make pnpm install about 4 seconds slower.

@ocavue ocavue changed the title Fix esbuild building chore: prefix main and module fields with ./ Jan 3, 2022
@ilkkao
Copy link
Contributor Author

ilkkao commented Jan 3, 2022

@ocavue great, makes sense of course that the fields are autogenerated. I won't touch this PR anymore. In the unlikely case a crude workaround to unblock would be needed for esbuild, I've used yarn install && rm -fr node_modules/jsdom. Jsdom is required inside a try/catch, so optional.

btw just switched from slate in my project and feel so much more productive now.

@ocavue ocavue merged commit 9eff813 into remirror:main Jan 3, 2022
@ocavue ocavue mentioned this pull request Jan 3, 2022
4 tasks
@ocavue
Copy link
Member

ocavue commented Jan 3, 2022

@ilkkao New version has been released. You might need to remove node_modules or even yarn.lock to update remirror to the latest version correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework: dom 🌐 Any changes to packages which are exclusively used for DOM-only codebases. framework: react ⚛️ Any changes to packages which are exclusively used for react code bases. package: jest-prosemirror 🎭 A change has occured in the jest-prosemirror package. package: jest-remirror 🃏 Label for the jest-remirror package package: multishift 🔽 Add this label when updating the `multishift` package. package: prosemirror-suggest 💡 A change has occurred in prosemirror-suggest. package: @remirror/cli 📟 A label for the remirror cli. type: changeset 🦋 A change has been made to the changesets folder. type: support 📚 A change focused on improving the architecture and structure of the support directory. type: ui 💅 Changes made to user facing components and styles.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants