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

"CommonJS or AMD" warnings #354

Closed
MickL opened this issue Mar 8, 2021 · 9 comments
Closed

"CommonJS or AMD" warnings #354

MickL opened this issue Mar 8, 2021 · 9 comments
Labels
date-fns issue Waiting for date-fns to fix an issue

Comments

@MickL
Copy link

MickL commented Mar 8, 2021

Question: Is it normal to have warnings like ...js depends on 'date-fns/...'. CommonJS or AMD dependencies can cause optimization bailouts. and is the supposed way to fix it to add date-fns to allowedCommonJsDependencies? If so, doesnt it make sense to add the instruction to the readme?

Also is there an open issue on date-fns project to build ecmascript modules?

@joanllenas
Copy link
Owner

It shouldn't be normal, date-fns provides ESM builds since v2.0.0.
https://date-fns.org/v2.0.0/docs/ECMAScript-Modules

@MickL
Copy link
Author

MickL commented Mar 8, 2021

If they do, I wonder why the compiler complains

joanllenas added a commit that referenced this issue Mar 8, 2021
Added esm to the import paths

#354
@joanllenas
Copy link
Owner

Ok, I've investigated a little bit more and it looks like imports need to have esm in their paths to get rid of those warnings. Like this: import addBusinessDays from 'date-fns/esm/addBusinessDays';
This is weird because the docs don't tell anything about it.

Anyway, I'm not sure if fixing esm will break ie11 applications, I will have to investigate a little more before releasing anything.

If you want to help try installing 8.0.1-beta.0 and let me know if you still see those warnings. If you have the chance to test on ie11 that would be great as well. I don't have a windows machine right now.

Cheers!

@MickL
Copy link
Author

MickL commented Mar 9, 2021

So i investigated a little bit into this and it seems like this is an issue of date-fns. Using 2.16.1 there is no warning and tree shaking works. Using >2.16.1 <= 2.19.0 there is a warning and FULL date-fns is imported.

So the warning is nice and correct and should not be ignored / suppressed by use of allowedCommonJsDependencies

Issue: date-fns/date-fns#2207

@MickL MickL closed this as completed Mar 9, 2021
@joanllenas
Copy link
Owner

That's good to know, thanks for taking the time to research this.
Cheers!

@jpduckwo
Copy link

date-fns/date-fns#2339
I think this will solve this issue in future builds once merged... maybe this should be left open until date-fns is resolved and it's confirmed working?

@joanllenas
Copy link
Owner

Thanks for pointing that out @jpduckwo , I will keep an eye on that PR.
And you are right, let's keep this opened for visibility until this is fixed.

Cheers!

@joanllenas joanllenas reopened this Mar 26, 2021
@joanllenas joanllenas added the date-fns issue Waiting for date-fns to fix an issue label Mar 26, 2021
@joanllenas
Copy link
Owner

FYI, a new version just came out that fixes the issue:
https://github.com/date-fns/date-fns/releases/tag/v2.20.3
I will give this a try during the weekend and if it works, will update the README explaining the proper lib. ranges and so on.

joanllenas added a commit that referenced this issue Apr 18, 2021
@joanllenas
Copy link
Owner

Tree shaking is fixed from date-fns v2.21.1 onwards.
Just updated the README clarifying that.
Thanks for your help,
cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
date-fns issue Waiting for date-fns to fix an issue
Projects
None yet
Development

No branches or pull requests

3 participants