search

joanllenas / ngx-date-fns

⏳ date-fns pipes for Angular
163 stars 14 forks source link

"CommonJS or AMD" warnings #354

Closed MickL closed 3 years ago

MickL commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

If they do, I wonder why the compiler complains

joanllenas commented 3 years ago

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 commented 3 years ago

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: https://github.com/date-fns/date-fns/issues/2207

joanllenas commented 3 years ago

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

jpduckwo commented 3 years ago

https://github.com/date-fns/date-fns/pull/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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