You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@metron.apache.org by ruffle1986 <gi...@git.apache.org> on 2018/10/03 13:27:26 UTC
[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
GitHub user ruffle1986 opened a pull request:
https://github.com/apache/metron/pull/1219
METRON-1796: [UI] Migrate off moment.js
## Contributor Comments
[DISCUSS] thread on the dev mailing list: https://lists.apache.org/thread.html/2e4fafa4256ce14ebcd4433420974e24962884204418ade51f0e3bfb@%3Cdev.metron.apache.org%3E
Original ASF Jira ticket: https://issues.apache.org/jira/browse/METRON-1796
The purpose of this PR is the complete removal of [moment.js](http://momentjs.com/) from the code base and replacing it with either native functions or [date-fns](https://date-fns.org/), another date manipulation library.
**Motivation**
In the Metron Alerts UI, we use a few functions only from moment.js to deal with time like formatting or displaying the relative time from "now". In order to do that, we have to import the entire library which causes a significant footprint in the compiled production bundle. Sometimes they can be replaced with native built-in solutions, sometimes they cannot but we can use date-fns which has a smaller size comparing to moment.js
As of date-fns v2 is in alpha phase, I rather chose the latest stable version which is 1.29.0. Tree-shaking is currently not supported in version 1 but [it will be in version 2](https://date-fns.org/v2.0.0-alpha.9/docs/ECMAScript-Modules). So once it's released and we can upgrade to it, we can see more size loss in the bundle size. Angular uses Webpack under the hood so if you're interested in what tree-shaking is and how Webpack does it, [follow this link](https://webpack.js.org/guides/tree-shaking/) for further details.
**Changes included**
- Every code used moment.js is replaced with a built-in function or the appropriate function from date-fns.
I highly recommend you to go through all the commits I've made one by one. They're straightforward and easier to understand which parts of the app are affected and in what way.
**Testing the bundle file size**
1. Checkout `master`
1. Go to `metron-interface/metron-alerts`
1. Make sure you have the latest packages in the `node_modules` folder via `npm ci`
1. Run `npm run build`
1. Take a look at the size of the `main.js` file
1. Checkout `ruffle1986/METRON-1796`
1. Run `npm ci` (this will install the date-fns library and remove moment.js)
1. Run `npm run build`
1. Take a look at the size of the `main.js` file and compare it to the number you've got in step 4
Here's the output of `npm run build` on the `master` branch:
And here's the output on this branch:
As you can see, by removing moment.js, we could reduce the bundle size to 1.65 MB which is **15.6%** comparing to the bundle with moment.js. Not so significant but still.
You've probably noticed the warning message on the second picture below the output of the build process. This is the result of compiling `pikaday-time` into our bundle via Angular. For the record, it's totally fine. Moment.js is just an optional dependency of [Pikaday](https://dbushell.com/Pikaday/), it works perfectly fine without it. This warning message is there because Pikaday [checks whether it's a Node.js environment and since it is, it wants to load moment from the node_modules folder](https://github.com/dbushell/Pikaday/blob/master/pikaday.js#L12-L16). However Pikaday doesn't throw and fails silently, the Angular compiler is clever enough to catch this and kindly warn you about it. But it doesn't cause any negative effect in the Metron UI because (again) moment.js is not a required dependency of Pikaday.
## Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our [Development Guidelines](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235) for the complete guide to follow for contributions.
Please refer also to our [Build Verification Guidelines](https://cwiki.apache.org/confluence/display/METRON/Verifying+Builds?show-miniview) for complete smoke testing guides.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
### For all changes:
- [X] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).
- [X] Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [X] Has your PR been rebased against the latest commit within the target branch (typically master)?
### For code changes:
- [X] Have you included steps to reproduce the behavior or problem that is being changed or addressed?
- [X] Have you included steps or a guide to how the change may be verified and tested manually?
- [X] Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
```
mvn -q clean integration-test install && dev-utilities/build-utils/verify_licenses.sh
```
- [X] Have you written or updated unit tests and or integration tests to verify your changes?
- [X] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [X] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
#### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that [travis-ci](https://travis-ci.org) is set up for your personal repository such that your branches are built there before submitting a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ruffle1986/metron METRON-1796
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/1219.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1219
----
commit 0848d6f777e987e1b91cda535594bd7da5d5c216
Author: ruffle1986 <ft...@...>
Date: 2018-09-28T09:25:55Z
cover timeRangeToDisplayStr with unit tests
commit 039edf4e3d00338280caea2e03267d626eff28da
Author: ruffle1986 <ft...@...>
Date: 2018-09-28T09:26:25Z
add date-fns as dependency
commit 47684ced0bb932e7de43433fa3bb9fa04fa198d1
Author: ruffle1986 <ft...@...>
Date: 2018-09-28T09:31:10Z
replace moment with date-fns in timeRangeToDisplayStr
commit 095600ea2204707b918899c9b782d4c30c6f9eac
Author: ruffle1986 <ft...@...>
Date: 2018-09-28T11:22:26Z
refactor(alert-details): use date-fns to calculate fromNow
commit fefeb7883fb3b35bad005e751a0c53e8cc5215be
Author: ruffle1986 <ft...@...>
Date: 2018-09-28T11:30:33Z
refactor(pcap-filters): use date-fns to format date
commit 2a077fc65c499c2acd16258d1d7920d805189b49
Author: ruffle1986 <ft...@...>
Date: 2018-09-28T11:57:55Z
refactor(date-picker): stop using moment.js
commit 716e0960f9f43257c32b6d9337ff17321c93f38f
Author: ruffle1986 <ft...@...>
Date: 2018-09-28T12:02:40Z
refactor(pcap-filters): remove moment.js and use date-fns in test
commit d95230c3de14417b868752b1fd6b4ac722d5468b
Author: ruffle1986 <ft...@...>
Date: 2018-09-28T12:14:46Z
refactor(time-lapse.pipe) remove moment.js and use date-fns
commit 1cff5107dd2fe5bc3097f149537f8a2f3fc92d81
Author: ruffle1986 <ft...@...>
Date: 2018-09-28T12:26:30Z
refactor(time-range): replace moment.js with date-fns (format)
commit 6f6fd6e67691591d32bf9d92cceff1407c8775f5
Author: ruffle1986 <ft...@...>
Date: 2018-10-01T14:10:19Z
chore(utils.spec): add license header
commit b57f535b363eca6cf595f01df819dac5e1a654aa
Author: ruffle1986 <ft...@...>
Date: 2018-10-03T10:34:54Z
test(alert-list): remove moment from e2e test
commit 2863708b54690570e76fdc6f261a7e647802cbe0
Author: ruffle1986 <ft...@...>
Date: 2018-10-03T10:35:44Z
chore: remove moment from package.json and add patched pikaday-time
commit e5d0fab6ba55edf400976b56f0e9fcc0029491cd
Author: ruffle1986 <ft...@...>
Date: 2018-10-03T10:36:25Z
refactor(date-picker): import patched pikaday-time
----
---
[GitHub] metron issue #1219: METRON-1796: [UI] Migrate off moment.js
Posted by tiborm <gi...@git.apache.org>.
Github user tiborm commented on the issue:
https://github.com/apache/metron/pull/1219
Thanks, Tamas! Reviewed and tested. +1
---
[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
Posted by ruffle1986 <gi...@git.apache.org>.
Github user ruffle1986 closed the pull request at:
https://github.com/apache/metron/pull/1219
---
[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
Posted by ruffle1986 <gi...@git.apache.org>.
Github user ruffle1986 commented on a diff in the pull request:
https://github.com/apache/metron/pull/1219#discussion_r223030359
--- Diff: metron-interface/metron-alerts/package.json ---
@@ -22,17 +22,17 @@
"@angular/platform-browser": "^6.1.6",
"@angular/platform-browser-dynamic": "^6.1.6",
"@angular/router": "^6.1.6",
+ "@ruffle1986/pikaday-time": "^1.6.1",
"@types/bootstrap": "^4.1.1",
"@types/jquery": "^3.3.4",
"ace-builds": "^1.2.6",
"ajv": "^6.5.1",
"angular-confirmation-popover": "^4.2.0",
"bootstrap": "4.0.0-alpha.6",
"core-js": "^2.4.1",
+ "date-fns": "^1.29.0",
"font-awesome": "^4.7.0",
- "moment": "^2.22.2",
"ng2-dragula": "^1.5.0",
- "pikaday-time": "^1.6.1",
--- End diff --
> Why is pikaday-time suddenly an issue?
I agree with you to not use smaller individual projects like `pikaday-time`. `pikaday-time` is introduced in the project by @iraghumitra almost a year ago.
The goal wasn't replacing `pikaday-time` with a better well-supported library but eliminating `moment.js` because of the aforementioned reasons in the description.
> I am concerned about this pikaday dependency. I would rather see us depending on larger, community supported projects like https://momentjs.com/, rather than smaller, individual supported projects like @owenmean/pikaday (or even your own fork @ruffle1986/pikaday-time).
`moment.js` is a very popular and great library to manipulate date and time.
`date-fns` has the exact same purpose.
`pikaday` is a standalone calendar user interface component.
`pikaday-time` is an extension of `pikaday` with additional fields to select time, not just date.
`moment.js` is an optional dependency of `pikaday` (! not `pikaday-time`). it works perfectly fine without `moment.js`. But if it is convenient to use `pikaday` with `moment`, it's fine then, you can use it by calling `pikaday`'s `getMoment` method if needed.
`pikaday-time` is just a fork of `pikaday` and the author of `pikaday-time` made a mistake by setting `moment.js` as a dependency of `pikaday-time` and every time we install `pikaday-time` we install `moment.js` too. If `moment.js` is installed in the `node_modules` folder `pikaday` will use it therefore it will be part of the bundle.
---
[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
Posted by ruffle1986 <gi...@git.apache.org>.
Github user ruffle1986 commented on a diff in the pull request:
https://github.com/apache/metron/pull/1219#discussion_r223984152
--- Diff: metron-interface/metron-alerts/package.json ---
@@ -22,17 +22,17 @@
"@angular/platform-browser": "^6.1.6",
"@angular/platform-browser-dynamic": "^6.1.6",
"@angular/router": "^6.1.6",
+ "@ruffle1986/pikaday-time": "^1.6.1",
"@types/bootstrap": "^4.1.1",
"@types/jquery": "^3.3.4",
"ace-builds": "^1.2.6",
"ajv": "^6.5.1",
"angular-confirmation-popover": "^4.2.0",
"bootstrap": "4.0.0-alpha.6",
"core-js": "^2.4.1",
+ "date-fns": "^1.29.0",
"font-awesome": "^4.7.0",
- "moment": "^2.22.2",
"ng2-dragula": "^1.5.0",
- "pikaday-time": "^1.6.1",
--- End diff --
Alright. I'm going to start a DISCUSS thread on the mailing list about it.
---
[GitHub] metron issue #1219: METRON-1796: [UI] Migrate off moment.js
Posted by ruffle1986 <gi...@git.apache.org>.
Github user ruffle1986 commented on the issue:
https://github.com/apache/metron/pull/1219
Since we are not there yet to merge it back, I'm closing this issue.
I can use this code later when we've solved all the prerequisites in order to satisfy the need of eliminating moment.js from the system.
---
[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/1219#discussion_r223502181
--- Diff: metron-interface/metron-alerts/package.json ---
@@ -22,17 +22,17 @@
"@angular/platform-browser": "^6.1.6",
"@angular/platform-browser-dynamic": "^6.1.6",
"@angular/router": "^6.1.6",
+ "@ruffle1986/pikaday-time": "^1.6.1",
"@types/bootstrap": "^4.1.1",
"@types/jquery": "^3.3.4",
"ace-builds": "^1.2.6",
"ajv": "^6.5.1",
"angular-confirmation-popover": "^4.2.0",
"bootstrap": "4.0.0-alpha.6",
"core-js": "^2.4.1",
+ "date-fns": "^1.29.0",
"font-awesome": "^4.7.0",
- "moment": "^2.22.2",
"ng2-dragula": "^1.5.0",
- "pikaday-time": "^1.6.1",
--- End diff --
@nickwallen - as an aside, I think this scenario that you've stumbled on here as it's been laid out shows precisely why the Metron community has steadily over time requested smaller and smaller PR's as well as more consistent use of DISCUSS threads when adding dependencies to the project. Having that pikaday fork is a fait accompli from a large PR that we're now having to pay the cost of living with.
@ruffle1986 Is there a reason we need to fork a project to get the time picker support? A couple other options here - Is it possible to either A, simply remove the time picker component or B, add a separate one that isn't part of pikaday or C, simply bring the relevant code into Metron so that if we need to maintain it or make changes we can feely do so? I'd prefer to simply not have it at all if it's an external dep that's been orphaned.
---
[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
Posted by ruffle1986 <gi...@git.apache.org>.
Github user ruffle1986 commented on a diff in the pull request:
https://github.com/apache/metron/pull/1219#discussion_r222315533
--- Diff: metron-interface/metron-alerts/package.json ---
@@ -46,9 +46,7 @@
"@types/ace": "0.0.32",
"@types/es6-promise": "0.0.33",
"@types/jasmine": "2.5.38",
- "@types/moment": "^2.13.0",
"@types/node": "~6.0.60",
- "@types/pikaday-time": "^1.4.2",
--- End diff --
The reason why I removed this is because moment.js is the dependency of this to make it able to add the type `moment` to the return value of the `getMoment` method. Therefore moment.js has to be inside the node_modules folder but we wan't to avoid it to be there.
I don't see any benefits of having the type definitions of pikaday-time in the code base.
---
[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1219#discussion_r223498638
--- Diff: metron-interface/metron-alerts/package.json ---
@@ -22,17 +22,17 @@
"@angular/platform-browser": "^6.1.6",
"@angular/platform-browser-dynamic": "^6.1.6",
"@angular/router": "^6.1.6",
+ "@ruffle1986/pikaday-time": "^1.6.1",
"@types/bootstrap": "^4.1.1",
"@types/jquery": "^3.3.4",
"ace-builds": "^1.2.6",
"ajv": "^6.5.1",
"angular-confirmation-popover": "^4.2.0",
"bootstrap": "4.0.0-alpha.6",
"core-js": "^2.4.1",
+ "date-fns": "^1.29.0",
"font-awesome": "^4.7.0",
- "moment": "^2.22.2",
"ng2-dragula": "^1.5.0",
- "pikaday-time": "^1.6.1",
--- End diff --
@ruffle1986 Your [PR](https://github.com/owenmead/Pikaday/issues/60) is unlikely to be accepted. The author has marked [@owenmean/pikeday](https://github.com/owenmead/pikaday) as deprecated.
IMO, I'd prefer us to just tackle option 1 first. Let's find a long-term replacement for @owenmean/pikeday that has broader community support and is not deprecated. After that, we can come back to Moment.js.
---
[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/1219#discussion_r223733562
--- Diff: metron-interface/metron-alerts/package.json ---
@@ -22,17 +22,17 @@
"@angular/platform-browser": "^6.1.6",
"@angular/platform-browser-dynamic": "^6.1.6",
"@angular/router": "^6.1.6",
+ "@ruffle1986/pikaday-time": "^1.6.1",
"@types/bootstrap": "^4.1.1",
"@types/jquery": "^3.3.4",
"ace-builds": "^1.2.6",
"ajv": "^6.5.1",
"angular-confirmation-popover": "^4.2.0",
"bootstrap": "4.0.0-alpha.6",
"core-js": "^2.4.1",
+ "date-fns": "^1.29.0",
"font-awesome": "^4.7.0",
- "moment": "^2.22.2",
"ng2-dragula": "^1.5.0",
- "pikaday-time": "^1.6.1",
--- End diff --
@ruffle1986 I agree with @nickwallen. I'd rather see us fix this fully than get 90% there and need to address a follow-on for it. Seeing as the point of this improvement is intended to improve the stability and maintainability of the UI, I think it makes sense to solve the whole problem in this case.
---
[GitHub] metron issue #1219: METRON-1796: [UI] Migrate off moment.js
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/1219
@ruffle1986 I just want to thank you for all the detail you put into your PR description and commentary to help ensure the community understands your work. Well done and it should serve as an example for all of us.
---
[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
Posted by ruffle1986 <gi...@git.apache.org>.
Github user ruffle1986 commented on a diff in the pull request:
https://github.com/apache/metron/pull/1219#discussion_r222308215
--- Diff: metron-interface/metron-alerts/package.json ---
@@ -22,17 +22,17 @@
"@angular/platform-browser": "^6.1.6",
"@angular/platform-browser-dynamic": "^6.1.6",
"@angular/router": "^6.1.6",
+ "@ruffle1986/pikaday-time": "^1.6.1",
"@types/bootstrap": "^4.1.1",
"@types/jquery": "^3.3.4",
"ace-builds": "^1.2.6",
"ajv": "^6.5.1",
"angular-confirmation-popover": "^4.2.0",
"bootstrap": "4.0.0-alpha.6",
"core-js": "^2.4.1",
+ "date-fns": "^1.29.0",
"font-awesome": "^4.7.0",
- "moment": "^2.22.2",
"ng2-dragula": "^1.5.0",
- "pikaday-time": "^1.6.1",
--- End diff --
pikaday-time is an extension of [Pikaday](https://github.com/dbushell/Pikaday). Pikaday is a great library with zero dependency. Pikaday-time is extends its functionality with time selection. However moment is just an optional dependency of Pikaday, there's an unpleasant side-effect in Pikaday-time where moment.js is set as a dependency in the package.json. So everytime we install pikaday-time, moment.js will be installed as well.
[I've tried to reach out the maintainer of the pikaday-time](https://github.com/owenmead/Pikaday/issues/60) library but he's not so active on Github and the library is no longer maintained anyway so it makes things a bit complicated. Hopefully we'll manage this and a patch for this will be introduced shortly.
Until then, [I've forked the pikaday-time repo](https://github.com/owenmead/Pikaday/compare/master...ruffle1986:master), made the changes and [published it on npm under my name](https://www.npmjs.com/package/@ruffle1986/pikaday-time). Originally I wanted to published it under [hortonworks](https://www.npmjs.com/org/hortonworks) but I don't know who have access to give me publish rights. The only change I made is setting moment.js as a `peerDependency` insteadOf setting it as an `optionalDependency`. Don't let the name `optionalDependency` mislead you. [It's basically a dependency but if it's cannot be found on npm on any given registries, npm install doesn't fail](https://docs.npmjs.com/files/package.json#optionaldependencies).
As soon as this issue is fixed and published on npm, we can remove the scoped pikaday-time and switch back to the original package.
---
[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
Posted by ruffle1986 <gi...@git.apache.org>.
Github user ruffle1986 commented on a diff in the pull request:
https://github.com/apache/metron/pull/1219#discussion_r223676139
--- Diff: metron-interface/metron-alerts/package.json ---
@@ -22,17 +22,17 @@
"@angular/platform-browser": "^6.1.6",
"@angular/platform-browser-dynamic": "^6.1.6",
"@angular/router": "^6.1.6",
+ "@ruffle1986/pikaday-time": "^1.6.1",
"@types/bootstrap": "^4.1.1",
"@types/jquery": "^3.3.4",
"ace-builds": "^1.2.6",
"ajv": "^6.5.1",
"angular-confirmation-popover": "^4.2.0",
"bootstrap": "4.0.0-alpha.6",
"core-js": "^2.4.1",
+ "date-fns": "^1.29.0",
"font-awesome": "^4.7.0",
- "moment": "^2.22.2",
"ng2-dragula": "^1.5.0",
- "pikaday-time": "^1.6.1",
--- End diff --
> A, simply remove the time picker component
If it's not important to let the user choose a time beside the date, then yes, we can get rid of `pikaday-time` and just use `pikaday`. Unfortunately there's a **but** here. :(
In order to eliminate moment, we still have to fork pikaday. Here's why:
[This](https://github.com/dbushell/Pikaday/pull/721) is a PR against Pikaday's master about removing moment.js from the dependencies. It's already merged but as it turned out, it's not published yet on npm. If you install the latest pikaday from npm, you will get moment as well. :( It's very difficult to follow and chaotic. This is a good example of choosing a 3rd party without proper investigation. And this is also the dangerous part of relying on 3rd party open source modules.
[Here's](https://github.com/dbushell/Pikaday/issues/805) an issue to push it, but no answers since 6th of September
> B, add a separate one that isn't part of pikaday
This would be the best imho because of the chaotic nature of pikaday. But it's hard to find a solution out there I could confidently rely on. The modules I trust are the [bootstrap datepicker](https://ng-bootstrap.github.io/#/components/datepicker/overview) or the [one by material design](https://material.angular.io/components/datepicker/overview). But I have my concerns about these. These are new dependencies and it has to be discussed which one should be picked. Also, introducing a datepicker by angular material would be weird imho, since it's very different from our current design and it wouldn't feel like a cohesive whole. But I'm not in the position to decide.
C, simply bring the relevant code into Metron so that if we need to maintain it or make changes we can feely do so?
This is why I've forked pikaday-time
---
[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1219#discussion_r223008966
--- Diff: metron-interface/metron-alerts/package.json ---
@@ -22,17 +22,17 @@
"@angular/platform-browser": "^6.1.6",
"@angular/platform-browser-dynamic": "^6.1.6",
"@angular/router": "^6.1.6",
+ "@ruffle1986/pikaday-time": "^1.6.1",
"@types/bootstrap": "^4.1.1",
"@types/jquery": "^3.3.4",
"ace-builds": "^1.2.6",
"ajv": "^6.5.1",
"angular-confirmation-popover": "^4.2.0",
"bootstrap": "4.0.0-alpha.6",
"core-js": "^2.4.1",
+ "date-fns": "^1.29.0",
"font-awesome": "^4.7.0",
- "moment": "^2.22.2",
"ng2-dragula": "^1.5.0",
- "pikaday-time": "^1.6.1",
--- End diff --
The core of this change is to move from moment.js to date-fns. Why is pikaday-time suddenly an issue?
I am concerned about this pikaday dependency. I would rather see us depending on larger, community supported projects like https://momentjs.com/, rather than smaller, individual supported projects like @owenmean/pikaday (or even your own fork @ruffle1986/pikaday-time).
Not only for continued support from obsolescence, but also because security vulnerabilities are all too common and our UI is a large attack surface. Larger communities means vulnerabilities are more likely to be uncovered and patched.
I get the technical motivation here. We want to decrease the load time. At the same time, we need to consider the organizations behind our dependencies to ensure their long-term viability and support.
Is there not another way we can tackle the technical challenge here?
---
[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
Posted by ruffle1986 <gi...@git.apache.org>.
Github user ruffle1986 commented on a diff in the pull request:
https://github.com/apache/metron/pull/1219#discussion_r223269377
--- Diff: metron-interface/metron-alerts/package.json ---
@@ -22,17 +22,17 @@
"@angular/platform-browser": "^6.1.6",
"@angular/platform-browser-dynamic": "^6.1.6",
"@angular/router": "^6.1.6",
+ "@ruffle1986/pikaday-time": "^1.6.1",
"@types/bootstrap": "^4.1.1",
"@types/jquery": "^3.3.4",
"ace-builds": "^1.2.6",
"ajv": "^6.5.1",
"angular-confirmation-popover": "^4.2.0",
"bootstrap": "4.0.0-alpha.6",
"core-js": "^2.4.1",
+ "date-fns": "^1.29.0",
"font-awesome": "^4.7.0",
- "moment": "^2.22.2",
"ng2-dragula": "^1.5.0",
- "pikaday-time": "^1.6.1",
--- End diff --
Correct.
---
[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
Posted by ruffle1986 <gi...@git.apache.org>.
Github user ruffle1986 commented on a diff in the pull request:
https://github.com/apache/metron/pull/1219#discussion_r222317156
--- Diff: metron-interface/metron-alerts/src/app/utils/utils.spec.ts ---
@@ -0,0 +1,215 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import {Utils} from './utils'
+
+describe('timeRangeToDisplayStr', () => {
--- End diff --
[timeRangeToDisplayStr](https://github.com/apache/metron/blob/master/metron-interface/metron-alerts/src/app/utils/utils.ts#L77-L206) relies heavily on moment.js so before touching the implementation I covered it with unit tests to make sure that the util produces the exact same result after the removal of moment.js.
---
[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1219#discussion_r223045213
--- Diff: metron-interface/metron-alerts/package.json ---
@@ -22,17 +22,17 @@
"@angular/platform-browser": "^6.1.6",
"@angular/platform-browser-dynamic": "^6.1.6",
"@angular/router": "^6.1.6",
+ "@ruffle1986/pikaday-time": "^1.6.1",
"@types/bootstrap": "^4.1.1",
"@types/jquery": "^3.3.4",
"ace-builds": "^1.2.6",
"ajv": "^6.5.1",
"angular-confirmation-popover": "^4.2.0",
"bootstrap": "4.0.0-alpha.6",
"core-js": "^2.4.1",
+ "date-fns": "^1.29.0",
"font-awesome": "^4.7.0",
- "moment": "^2.22.2",
"ng2-dragula": "^1.5.0",
- "pikaday-time": "^1.6.1",
--- End diff --
Is this a fair statement of the facts? I want to make sure I understand.
* The [@owenmean/pikeday](https://github.com/owenmead/pikaday) dependency is not a new dependency for us
* [@owenmean/pikeday](https://github.com/owenmead/pikaday) is a fork of a popular library called [pikaday](https://github.com/dbushell/Pikaday) that has some enhancements we use
* [@owenmean/pikeday](https://github.com/owenmead/pikaday) is a deprecated library that is no longer maintained
* The only reason we had to touch the [@owenmean/pikeday](https://github.com/owenmead/pikaday) dependency is because it pulls in MomentJS
* There are only two ways to exclude MomentJS from being pulled in by [@owenmean/pikeday](https://github.com/owenmead/pikaday)
* Option 1: Find a long-term replacement for [@owenmean/pikeday](https://github.com/owenmead/pikaday) that has broader community support
* Option 2: Directly forking it to [@ruffle1986/pikaday-time](https://github.com/ruffle1986/pikaday) and excluding MomentJS
* As part of this PR you chose to go with option 2, favoring expediency.
* You think we should do option 1 (find a long-term replacement for [@owenmean/pikeday](https://github.com/owenmead/pikaday)) as a separate, follow-on PR.
---