-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(module): export saveAs method and define module entrypoint #602
base: master
Are you sure you want to change the base?
Conversation
So, I don't think this PR will be accepted because you're manipulating a dist output coming from babel transform. The underlying problem here is that the babel transform to UMD is messed up with respect to exports. This is discussed here: babel/babel#10696. You probably need to put pressure on the babel group to fix their library. |
@chrisknoll the updated dist output is generated by babel, I did not edit it by hand (I might be wrong though... this PR is very old). |
Understood, Thanks. I see it now, your addition of 'export saveAs' probably led to the change. |
Angular 10 is putting pressure on moving from Common JS to ES modules, could we for example put this pr as separate version of filesaver.js just like with lodash-es? |
Why this is not yet merged in? It is really helpful feature. |
+1 Migrating from Angular 8 to Angular 10. |
Is there any intension for this to be merged, or even for the creation of a seperate version as mentioned by @criskrzysiu |
Any update? Would be cool to eliminate the warning: |
It's causing fail in my app, any updates? |
Is this something that we just need to rely on a fork to resolve? Unfortunate, but what else can be done? |
Any update? I have the same warning: CommonJS or AMD dependencies can cause optimization bailouts. |
@cristinecula thanks for doing the legwork on this. Hopefully it gets merged soon. In the meantime, I thought I'd share an easy way to install this PR in your node modules folder for us script kiddies out there:
then it should be easy to replace once this package gets updated. [edit] linked the wrong package. sorry! |
Our dependencies are loaded via package.json. Can I specify that information in the package dependencies? Making all our consumers of our library run that command manually for their local installs is not feasible. |
Sorry about that, I @'d the wrong person. To answer your question that line should add the fork to your package.json dependencies. But, if you have any more questions regarding installing packages from github I highly suggest using stack overflow so this thread can remain on topic :) |
In all fairness, you brought it up.... |
I stumbled across https://www.npmjs.com/package/file-saver-es which seems to address this issue. Not sure how "official" this package is however. |
@eligrey As the sole owner of this repo, can you have a look at this PR. It will help the whole Angular community if this gets merged. |
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.
Needs a version bump
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.
lgtm after rebuilding to resolve conflicts 👍
@eligrey Since you approved the PR, could you merge and publish a new version ? It has been 3 years since the PR has been opened and it's still an issue.
|
@eligrey @cristinecula could you give an update on this? |
@cristinecula can you please resolve the branch conflicts? |
@eligrey is there any update on this? it's been a while... :/ |
Fixes #538
This enables code using es modules to do:
import {saveAs} from 'file-saver'
Legacy js code is not affected by this change.