[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
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
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 pull request #1219: METRON-1796: [UI] Migrate off moment.js
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 pull request #1219: METRON-1796: [UI] Migrate off moment.js
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
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
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
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
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. ---
[GitHub] metron pull request #1219: METRON-1796: [UI] Migrate off moment.js
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
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
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
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
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
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- where is the JIRA number you are trying to resolve? Pay particular attention to the
