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

Adding exports map in package.json #3218

Closed
7 tasks
sidharthv96 opened this issue Jan 31, 2024 · 11 comments
Closed
7 tasks

Adding exports map in package.json #3218

sidharthv96 opened this issue Jan 31, 2024 · 11 comments
Labels
bug A bug in the code of Cytoscape.js stale

Comments

@sidharthv96
Copy link

Environment info

  • Cytoscape.js version : 3.28.1
  • Browser/Node.js & version : node v20

Current (buggy) behaviour

When using import cytoscape from 'cytoscape'; and bundling the code in a library, an regarding importing lodash is thrown during build.

Desired behaviour

It should import the right bundle. This can be facilitated by adding an exports map.

https://github.com/mermaid-js/mermaid/pull/5247/files#diff-f0ed092719ff4df1eac0ddfe6e5cf61e979cbd3bae566ed14cb579290735faa8

Minimum steps to reproduce

(I can create the STR if mandatory, I can raise the PR directly with the fix, as the patch provided above is working for us)

What do you need to do to reproduce the issue?

Fork/clone this JSBin demo and reproduce your issue so that your issue can be addressed quickly and effectively:
http://jsbin.com/fiqugiq

For reviewers

Reviewers should ensure that the following tasks are carried out for incorporated issues:

  • Ensure that the reporter has included a reproducible demo. They can easily fork this JSBin demo: http://jsbin.com/fiqugiq
  • The issue has been associated with a corresponding milestone.
  • The commits have been incorporated into the corresponding branches. Bug-fix patches go on
    • master,
    • unstable, and
    • the previous feature release branch (e.g. 1.1.x if the current release is 1.2).
  • The issue has been labelled as a bug, if necessary.
@sidharthv96 sidharthv96 added the bug A bug in the code of Cytoscape.js label Jan 31, 2024
@sidharthv96
Copy link
Author

sidharthv96 commented Jan 31, 2024

In the export map, using the ESM version (./dist/cytoscape.esm.js)also throws similar error.

@maxkfranz
Copy link
Member

Should be superseded by:

@maxkfranz
Copy link
Member

See:

@sidharthv96, would you follow the steps in #3217 (comment) to confirm that the exports map works in your project?

@sidharthv96
Copy link
Author

@maxkfranz, sadly that didn't work.

"lodash/debounce" is imported by "lodash/debounce?commonjs-external", but could not be resolved – treating it as an external dependency.
"lodash/get" is imported by "lodash/get?commonjs-external", but could not be resolved – treating it as an external dependency.
"lodash/set" is imported by "lodash/set?commonjs-external", but could not be resolved – treating it as an external dependency.
"lodash/toPath" is imported by "lodash/toPath?commonjs-external", but could not be resolved – treating it as an external dependency.

This is what worked in mermaid.

diff --git a/package.json b/package.json
index f2f77fa79c99382b079f4051ed51eafe8d2379c8..0bfddf55394e86f3a386eb7ab681369d410bae07 100644
--- a/package.json
+++ b/package.json
@@ -30,7 +30,15 @@
   "engines": {
     "node": ">=0.10"
   },
+  "exports": {
+    ".": {
+      "import": "./dist/cytoscape.umd.js",
+      "default": "./dist/cytoscape.cjs.js"
+    },
+    "./*": "./*"
+  },
   "main": "dist/cytoscape.cjs.js",
+  "module": "dist/cytoscape.umd.js",
   "unpkg": "dist/cytoscape.min.js",
   "jsdelivr": "dist/cytoscape.min.js",
   "scripts": {

@maxkfranz
Copy link
Member

You need to make a build of Cytoscape. UMD really shouldn’t be used for import.

maxkfranz added a commit that referenced this issue Feb 27, 2024
- Improving ESM support and documentation, esp. for browsers #3217
- Adding exports map in package.json #3218
@maxkfranz
Copy link
Member

maxkfranz commented Feb 27, 2024

I've updated the dist files on the unstable branch: https://github.com/cytoscape/cytoscape.js/blob/4372a14065ed26aa781f3b563d5907b9fee80054/dist/cytoscape.esm.js

That should make your testing a bit simpler. You can verify that the ESM file linked above doesn't use any import statements whatsoever.

Steps:

  1. git checkout git@github.com:cytoscape/cytoscape.js.git
  2. cd cytoscape.js
  3. git checkout unstable && git pull
  4. npm link
  5. cd ../my-app
  6. npm link cytoscape
  7. npm run watch or npm run dev or npm run build -- however you do it
  8. Verify your app builds and works

@sidharthv96
Copy link
Author

@maxkfranz tried it. Didn't work. I forgot to update here then. Will rerun and add the logs.

maxkfranz added a commit that referenced this issue Mar 6, 2024
Parts of lodash are used:
- lodash/get
- lodash/set
- lodash/toPath
- lodash/debounce

`heap` is used, but it hasn't been updated in a long time.  It probably never needs to be updated unless there's a serious bug.  Doubtful.

Ref:
- Improving ESM support and documentation, esp. for browsers #3217
- Adding exports map in package.json #3218
@maxkfranz
Copy link
Member

@sidharthv96

All of the builds on unstable currently have no external dependencies, including CJS now. There is nothing listed under package.json > dependencies anymore, either. If linking to the unstable branch doesn't work with the testing steps, then I suspect that something is off with your build config -- or the npm link step.

You'll need to delete your patch file in Mermaid to test the steps.

@sidharthv96
Copy link
Author

@maxkfranz it's working perfectly now! Thank you!

We also moved to using esbuild as well.

image

Now, one of our major concern is the size. I'll try to see if there are any low hanging fruits related to tree shaking available in cytoscape.

@maxkfranz
Copy link
Member

maxkfranz commented Mar 8, 2024

@maxkfranz it's working perfectly now! Thank you!

Great

We also moved to using esbuild as well.

Also great

image

Now, one of our major concern is the size. I'll try to see if there are any low hanging fruits related to tree shaking available in cytoscape.

Cytoscape does a lot. It has a CSS parser, a renderer, several layouts, etc. There's a lot there.

This probably couldn't be changed that much in the current v3 line in the main builds. Possibly in v4. It would probably require some significant API changes re. extensions.

For your case, you're welcome to add a Rollup build config to the Cytoscape repo that's smaller, if you like. It could exclude things like the canvas renderer if you're just using it headlessly for the model and layouts. Something like 'cytoscape.headless.esm.js'.

Note you'd have to do the following in Mermaid, if you use a custom build:

import cytoscape from 'cytoscape/dist/cytoscape.headless.esm.js'

Copy link

stale bot commented Mar 22, 2024

This issue has been automatically marked as stale, because it has not had activity within the past 14 days. It will be closed if no further activity occurs within the next 7 days. If a feature request is important to you, please consider making a pull request. Thank you for your contributions.

@stale stale bot added the stale label Mar 22, 2024
@stale stale bot closed this as completed Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug in the code of Cytoscape.js stale
Projects
None yet
Development

No branches or pull requests

2 participants