[ADS-645] Remove extra chunk and make moment external

[sonhanguyen]

Description

• Remove the extra bundle
• Externalizing momentjs

Motivation and Background Context

• Currently, the build generates 3 bundles: "core", "extra" (that contains react-datepicker) and "main" that has everything. The splitting was done mainly because react-datepicker has momentjs bundled which we don't use but Symphony does, and moment's size is considerable.
• direct-web however has not switched to only import core, so moment code is duplicated anyway.

Does this PR introduce a breaking change?

for SYMPHONY in case they don't already have moment as a dependency, If they do this saves them 240K as well

• Yes
• No

How Has This Been Tested?

Screenshots (if appropriate):

Check-list:

• I have read the Contributing document.
• I've thought about and labelled my PR/commit message appropriately.
• If this PR introduces breaking changes I've described the impact and migration path for existing applications.
• CI is green (coverage, linting, tests).
• I have updated the documentation accordingly.
• I've two LGTMs/Approvals.
• I've fixed or replied to all my code-review comments.
• I've manually tested with a buddy.
• I've squashed my commits into one.

[codecov-io]

Codecov Report

Merging #686 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #686   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          53     53           
  Lines         776    776           
  Branches      154    154           
=====================================
  Hits          776    776

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 072b1de...2c49eb6. Read the comment docs.

[omgaz]

The issue wasn't with moment it was with moment-tz, and where further breaking changes may appear. Have a look at https://github.com/Hacker0x01/react-datepicker#localization and check localisations.

[omgaz]

Apart from the comment around localisation, LGTM.

I just have a concern about externalising moment. It's a clear dependency of our code, unlike react which is the framework our code is built upon. I know it saves us some size but it's now required to be installed by a third-party application and not sure if it makes sense (unlike moment-tz) like my last comment mentions.

I still feel it should be an installed dependency.

[sonhanguyen]

@omgaz I don't see many problems because moment would be the in same line to lodash by that logic. react-datepicker externalizes it too as a peerDependencies. We initially did import 'react-datepicker/dist/react-datepicke' which wasn't the intended way, that's why it was included.

[nimishjha]

The code looks fine, but I'm not familiar with the background here.

[omgaz]

@sonhanguyen did you send this around for another review?

[xiaofan2406]

👍

[xiaofan2406]

by removing these extra entries, you can revert changes made in #672 (see its description)

[sonhanguyen]

reverted

[xiaofan2406]

LGTM

[sonhanguyen]

@omgaz @xiaofan2406, should we make moment an optional dependency?

[xiaofan2406]

It should be a peerDeps, like react

[sonhanguyen]

@xiaofan2406 yeah but peer means required. We happen to have moment in direct-web too, but we didn't include moment because of adslot-ui. We don't actually use its datepicker component.

[sonhanguyen]

@omgaz what do you think about react as a peerDep, seems like the way most projects do it.

[omgaz]

moment is a requirement so it does make sense for it to be a peer dependency like react, but we can deal with it in another PR.

Also, we do use the react-datepicker in all projects.

[xiaofan2406]

I remember peer deps will come off as warning not error.

So if a project uses adslot-ui and dont use the date picker component, the project can choose to not to install moment, and other components should be working correctly

[omgaz]

LGTM, will merge in a bit. @sonhanguyen looks like we need a rebase; sorry.

[omgaz]

Rebased for release.