-
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
Added proper support for ES module exports (with some fixes for NodeJS context) #613
Conversation
Hey @timocov Do you have any updates on this? |
@cipriancaba not yet, we're waiting for changes in |
Appreciate the update |
@timocov tradingview/fancy-canvas#7 merged, can we merge this PR too? |
@zouxuoz this PR leads for breaking change in the package so we cannot merge right now since we're preparing the next minor update for the library. I think after this version we can start to prepare the next major version with all breaking change we're going to do. |
@timocov do you have ETA for it? 😔 can I help? |
Hey @timocov, Hope all is well, I saw your comment here: https://stackoverflow.com/a/65230044 in regards to the below error in a nodeJS context: import { bindToDevicePixelRatio } from 'fancy-canvas/coordinate-space';
^^^^^^
SyntaxError: Cannot use import statement outside a module I was wondering if there is any update to this at all? Specifically, being able to create charts and save image to file. Thank you in advance, Gabe |
@Zidious we're planning to fix the issue in v4. We didn't do it before because it might be a breaking change for users. |
02062bc
to
8f200cc
Compare
…n nodejs context It's the case to handle that the library might be used with SSR
It seems that in fancy-canvas we have a bug that it's impossible to import it in modules. To merge this branch we're waiting for:
|
I believe this PR is ready to be rebased and updated accordingly the latest changes regarding the docs (but I have no access to the branch anymore so fyi lwc maintainers). |
FYI: Results of running
|
website/plugins/enhanced-codeblock/theme/CodeBlock/import-lightweight-charts-version.ts
Outdated
Show resolved
Hide resolved
`npm run verify` needed to be changed so that the build script is finished before eslint starts else there will be an error reported within `import-lightweight-charts-version.ts`
@Nipheris I've added back |
The standalone esm version would allows users to use the import syntax directly in the browser without a build step. <script type="module">
import { createChart } from './dist/lightweight-charts.standalone-esm.production.js';
// or import { createChart } from 'https://unpkg.com/lightweight-charts/dist/lightweight-charts.standalone-esm.production.js';
var chart = createChart(document.getElementById('container'));
// ...
</script>
|
Tbh from my perspective I don't think you need to even handle this case - nodejs supports modules since v16 (right?) which is current LTS (nodejs 14 will shut down in 2 months https://nodejs.dev/en/about/releases/), all modern bundles should support exports map or some sort of. Looks like unnecessary work for you :) But it is up to you ofc 😉 |
In this case, it was very little effort to add the Whilst we do specify node 16 in the @timocov Obviously we value your opinion and if you think it would be better to exclude these builds and fallback then we take another look at this. |
Afaik it is removed from published
Adding Another thought was that this package was (still is?) unusable in nodejs context at all as it requires lots of browser-specific API. The only purpose of having it passing "being imported in nodejs" is to make SSR happy that is widely used by some frameworks. But they cannot use this package now in SSR because of issues anyway. And most likely these frameworks require modern-enough nodejs for work anyway or process imported files differently without relying on pure nodejs require/import mechanism (e.g. as they need to handle imports of css modules as well as others). And keep in mind that nodejs 14 is LTS for 2 months since now and I assume maintainers of other packages/frameworks might start to migrate their min nodejs versions to 16. But obviously it requires having an understanding of your customers and whether you can or cannot drop cjs support for them in the next major release, which I cannot have unlike you.
Yes, this is very fair point 👍 @SlicedSilver And please don't consider any of my words as nothing more as just an opinion of another random from the Internet. I'm not a maintainer anymore and out of context for a long time and I'm not in position to tell you what to do. And obviously I don't want you to change your mind 🙂 At the end you'll be maintaining this project after any merged changes 😂 At this point I'm shutting up in this thread 😂 🤐 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Maybe we could also improve Installing
section in README.md
, to be clear about available alternatives. To summarize, now we have the following:
ES module | CommonJS | IIFE (window.LightweightCharts ) |
||
---|---|---|---|---|
Deps NOT included | PROD | production.mjs |
production.cjs |
N/A |
Deps NOT included | DEV | development.mjs |
development.cjs |
N/A |
Deps included (standalone) | PROD | standalone.production.mjs |
- | standalone.production.js |
Deps included (standalone) | DEV | standalone.development.mjs |
- | standalone.development.js |
Type of PR: enhancement
PR checklist:
Overview of changes:
This PR includes a test in CI to make sure that you're able to import
lightweight-charts
package in NodeJS context,exports
map inpackage.json
(including different files for dev/prod builds 🎉 - now it should work out of the box almost for everybody), the publishedpackage.json
file will have"type": "module"
.