-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Improving ESM support and documentation, esp. for browsers #3217
Comments
Good analysis. What about absorbing all the dependencies? They're small and it would simplify the bundles -- i.e. '.min.js' would be the same as '.js' w.r.t. dependencies. You also wouldn't need to specify a mapping. PRs welcome. |
I'm working on transpiling dagre right now to ESM.
It's cytoscape --> dagre-cytoscape --> dagre --> graphlib.
I'm almost done with graphlib.
Why would min currently be different then non-min for deps? Something else
for me to investigate...
…On Wed, Jan 31, 2024 at 9:12 AM Max Franz ***@***.***> wrote:
Good analysis.
What about absorbing all the dependencies?
They're small and it would simplify the bundles -- i.e. '.min.js' would be
the same as '.js' w.r.t. dependencies. You also wouldn't need to specify a
mapping.
PRs welcome.
—
Reply to this email directly, view it on GitHub
<#3217 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHHLRFOI6FGSUR7LVRN4M3DYRJGNPAVCNFSM6AAAAABCSEJUMCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJZGE4DGNRXG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
The min builds include the dependencies within the one file. |
Oh so even in ESM you bundle them 🤔. I'm not sure I would expect that behavior as an end-user. Resolved by your suggestion though! I will see maybe if I get around to it. Thanks! |
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. |
Tested https://github.com/cytoscape/enrichment-map-webapp
@mikekucera, @chrtannus -- this is an important but potentially disruptive change. Would you test the steps above and confirm that the EM build still works for you? I'd like to confirm that this is working on machines other than my Macbook. |
@ayjayt, would you also confirm the steps in the previous comment -- replacing |
I can do it before Monday |
I've updated the That should make your testing a bit simpler. You can verify that the ESM file linked above doesn't use any Steps:
|
These steps only work if the import cytoscape from 'cytoscape'; Note that only the default may be used. So this would also work: import { default as cytoscape } from 'cytoscape' Note: This will not work: import * as cytoscape from 'cytoscape' |
Documentation updates: https://js.cytoscape.org/#getting-started/including-cytoscape.js
This example should be:
Around this text, there should be a note that |
@ayjayt, how did it go with your build? |
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
Hey sorry, had some good news over here that I had to act on, I'm free now to do it this weekend! |
My app is 100% web so that buildflow doesn't work for me- but I did set up a test project to see if I could I get the "you must include
My least favorite thing about ESM. There are some strong opinions floating around about it. But yes, easiest is to recommend NOT to use
Is this true? AFAIK there is no automatic global scope when using In my app, I have to use the original syntax. |
I will note that it doesn't work if I add Also, I've never seen Nitpicking here |
Released in 3.29.x |
Update the package to better support ESM, especially in the browser, by default:
.esm.js
build, which removes the need for specifying import maps. They are small dependencies, and other builds already include them (like the UMD build).package.json
to includemodule
, pointing to the ESM build.exports
key inpackage.json
forimport
andrequire
.The
package.json
updates for ESM will work in the browser, in bundlers like Webpack, and in Node, because it doesn't have any external dependencies.Original post follows:
I made it work. This about using cytoscape esm or ANY esm in the browser WITHOUT bundling, which in cytoscape is currently brokenish.
You have 2 dependencies, 1 of which,
lodash
, you split into a few.Browser only supports ESM w/ relative imports because they haven't figured out how they want to handle non-URI indicators like
import { myFunc } as "SomeWebGlobalModule"
.So you use an inline
<script type=importmap>
in<head>
, mine looks like this:(no
src
available, only one declaration, first thing).I also had to change the cytoscape ESM file:
Now, with esm, because of static analysis, you don't need to split lodash off into separate things to make a bundler understand not to use the whole library. It can shake the tree. The network doesn't shake trees tho, and so we pull the whole file over the network. I wanted to use a CDN, and I didn't find esm versions of the individual methods, only the whole lodash.
Heap is an unmaintained library, 3 years now. Its also like 80 lines of code and you should probably just absorb it. I did fix it to support
esm
qiao/heap.js#34 (there is no way to import a cjs-only package except in node or webpack or rollup or etc which understandrequire
because the import is effectively transpiled torequire
since that's all it supports.)My suggestions for better supporting ESM:
importmap
w/o heap and also change your_esm
file for my combined import.or
importmap
.Hi @TpmKranz and @maxkfranz . Just @ ing you because I see you on the git-blame for how to use ESM w/ browser :-)
Thanks!
ps. thinking about making a pull request to
npm
likenpm install --web
which could solve some of these issues.The text was updated successfully, but these errors were encountered: