You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2021/06/02 09:53:11 UTC
[GitHub] [guacamole-client] mike-jumper opened a new pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
mike-jumper opened a new pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616
This change updates absolutely all JavaScript dependencies to the latest. Unfortunately, with additional transitive dependencies, doing this required migrating to npm and webpack for the frontend part of the building/bundling process. WebJars was just too eager to include transitive dependencies that aren't truly used, whereas webpack will magically determine such things.
With JavaScript dependencies no longer 100% coming from Maven, I had to write a webpack plugin to walk through the contents of the bundle, determine which dependencies are present, and dump a list of those dependencies to a file for automated license processing.
It's not as huge a change as it initially appears. Most of the modified files are renames, and most of the lines are `package-lock.json`. Beware: just like the basilisk, `package-lock.json` is controlled by magic and looking upon it with your own eyes can be fatal.
I'm opening this as a draft for now, as I need to perform some broader regression tests, but here it is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#issuecomment-853539164
Well, I think this all looks pretty reasonable to me. Let's do it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#discussion_r644459752
##########
File path: guacamole/src/main/frontend/src/app/index/indexModule.js
##########
@@ -17,22 +17,37 @@
* under the License.
*/
+require('angular-module-shim.js');
+require('relocateParameters.js');
+
+require('angular-translate-interpolation-messageformat');
+require('angular-translate-loader-static-files');
+
/**
* The module for the root of the application.
*/
angular.module('index', [
+
+ require('angular-route'),
Review comment:
These are the external AngularJS libraries that we depend on. It's a convention with AngularJS libraries within NPM to simply export the name of the relevant AngularJS module, so `require('angular-route')` here returns a JavaScript string that is the name of the AngularJS module providing routing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a change in pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on a change in pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#discussion_r644459719
##########
File path: guacamole/src/main/frontend/src/app/index/indexModule.js
##########
@@ -17,22 +17,37 @@
* under the License.
*/
+require('angular-module-shim.js');
+require('relocateParameters.js');
+
+require('angular-translate-interpolation-messageformat');
+require('angular-translate-loader-static-files');
+
/**
* The module for the root of the application.
*/
angular.module('index', [
+
+ require('angular-route'),
+ require('angular-translate'),
+
'auth',
'client',
'clipboard',
'home',
'login',
'manage',
'navigation',
- 'ngRoute',
- 'ngTouch',
'notification',
- 'pascalprecht.translate',
'rest',
'settings',
+
Review comment:
Makes sense to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a change in pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on a change in pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#discussion_r644458719
##########
File path: guacamole/src/main/frontend/src/app/index/indexModule.js
##########
@@ -17,22 +17,37 @@
* under the License.
*/
+require('angular-module-shim.js');
+require('relocateParameters.js');
+
+require('angular-translate-interpolation-messageformat');
+require('angular-translate-loader-static-files');
+
/**
* The module for the root of the application.
*/
angular.module('index', [
+
+ require('angular-route'),
Review comment:
Why are these here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner merged pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
jmuehlner merged pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#issuecomment-853442974
I think this is now ready!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#issuecomment-853442318
OK - I've gone through the [AngularJX 1.6 -> 1.7 migration guide](https://docs.angularjs.org/guide/migration#migrating-from-1-6-to-1-7) and the only applicable change is with the ngTouch module (angular-touch). We used to use this module to remove the ~300ms click delay on touch devices, but the new version ngTouch no longer does this.
This should still be OK - as noted in the migration guide, modern browsers and even IE10+ no longer apply that delay. With that no longer being an issue, and with support for working around the issue gone, there's no need to include ngTouch as a dependency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#discussion_r644451733
##########
File path: guacamole/src/main/frontend/package.json
##########
@@ -0,0 +1,35 @@
+{
+ "private": true,
+ "scripts": {
+ "build": "webpack --progress"
+ },
+ "dependencies": {
+ "@simonwep/pickr": "1.8.1",
+ "angular": "1.8.2",
+ "angular-route": "1.8.2",
+ "angular-templatecache-webpack-plugin": "^1.0.1",
Review comment:
Reading up on this a bit, it seems the best approach is to use `^` except where there's a specific reason not to:
* It's the default for npm and therefore the least surprising to anyone already well-steeped in that ecosystem.
* It avoids duplicating the duty of `package-lock.json`, which we are including in version control here following the _other_ established best practice for npm.
I definitely prefer the Maven approach of absolute reproducibility by default (with the Maven equivalent to `^` and `~` being considered bad practice and almost never used), but we're stuck with the npm approach here.
I'll update to 🥕s everywhere.
##########
File path: guacamole/src/main/frontend/src/app/index/indexModule.js
##########
@@ -17,22 +17,37 @@
* under the License.
*/
+require('angular-module-shim.js');
+require('relocateParameters.js');
+
+require('angular-translate-interpolation-messageformat');
+require('angular-translate-loader-static-files');
+
/**
* The module for the root of the application.
*/
angular.module('index', [
+
+ require('angular-route'),
+ require('angular-translate'),
+
'auth',
'client',
'clipboard',
'home',
'login',
'manage',
'navigation',
- 'ngRoute',
- 'ngTouch',
'notification',
- 'pascalprecht.translate',
'rest',
'settings',
+
Review comment:
I thought it made sense to visually separate things into three groups:
* AngularJS modules from external libraries
* AngularJS modules we define ourselves with actual code
* AngularJS modules that are generated at build time (just `templates-main`)
##########
File path: guacamole/src/main/frontend/src/app/index/indexModule.js
##########
@@ -17,22 +17,37 @@
* under the License.
*/
+require('angular-module-shim.js');
Review comment:
Yep, exactly.
##########
File path: guacamole/src/main/frontend/src/app/index/indexModule.js
##########
@@ -17,22 +17,37 @@
* under the License.
*/
+require('angular-module-shim.js');
+require('relocateParameters.js');
+
+require('angular-translate-interpolation-messageformat');
+require('angular-translate-loader-static-files');
+
/**
* The module for the root of the application.
*/
angular.module('index', [
+
+ require('angular-route'),
Review comment:
These are the external AngularJS libraries that we depend on. It's a convention with AngularJS libraries within NPM to simply export the name of the relevant AngularJS module, so `require('angular-route')` here returns a JavaScript string that is the name of the AngularJS module providing routing.
##########
File path: guacamole/src/main/frontend/plugins/dependency-list-plugin.js
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ */
+
+const finder = require('find-package-json');
+const fs = require('fs');
+const path = require('path');
+const validateOptions = require('schema-utils');
+
+/**
+ * The name of this plugin.
+ *
+ * @type {string}
+ */
+const PLUGIN_NAME = 'dependency-list-plugin';
+
+/**
+ * The schema of the configuration options object accepted by the constructor
+ * of DependencyListPlugin.
+ *
+ * @see https://github.com/webpack/schema-utils/blob/v1.0.0/README.md#usage
+ */
+const PLUGIN_OPTIONS_SCHEMA = {
+ type: 'object',
+ properties: {
+
+ /**
+ * The name of the file that should contain the dependency list. By
+ * default, this will be "npm-dependencies.txt".
+ */
+ filename: { type: 'string' },
+
+ /**
+ * The path in which the dependency list file should be saved. By
+ * default, this will be the output path of the Webpack compiler.
+ */
+ path: { type: 'string' }
+
+ },
+ additionalProperties: false
+};
+
+/**
+ * Webpack plugin that automatically lists each of the NPM dependencies
+ * included within any bundles produced by the compile process.
+ */
+class DependencyListPlugin {
+
+ /**
+ * Creates a new DependencyListPlugin configured with the given options.
+ * The options given must conform to the options schema.
+ *
+ * @see PLUGIN_OPTIONS_SCHEMA
+ *
+ * @param {*} options
+ * The configuration options to apply to the plugin.
+ */
+ constructor(options = {}) {
+ validateOptions(PLUGIN_OPTIONS_SCHEMA, options, 'DependencyListPlugin');
+ this.options = options;
+ }
+
+ /**
+ * Entrypoint for all Webpack plugins. This function will be invoked when
+ * the plugin is being associated with the compile process.
+ *
+ * @param {Compiler} compiler
+ * A reference to the Webpack compiler.
+ */
+ apply(compiler) {
+
+ /**
+ * Logger for this plugin.
+ *
+ * @type {Logger}
+ */
+ const logger = compiler.getInfrastructureLogger(PLUGIN_NAME);
+
+ /**
+ * The full path to the output file that should contain the list of
+ * discovered NPM module dependencies.
+ *
+ * @type {string}
+ */
+ const outputFile = path.join(
+ this.options.path || compiler.options.output.path,
+ this.options.filename || 'npm-dependencies.txt'
+ );
+
+ // Wait for compilation to fully complete
+ compiler.hooks.done.tap(PLUGIN_NAME, (stats) => {
+
+ const moduleCoords = {};
+
+ // Map each file used within any bundle built by the compiler to
+ // its corresponding NPM package, ignoring files that have no such
+ // package
+ stats.compilation.fileDependencies.forEach(file => {
+
+ // Locate NPM package corresponding to file dependency (there
+ // may not be one)
+ const moduleFinder = finder(file)
Review comment:
🙀 Oops
##########
File path: guacamole/src/main/frontend/plugins/dependency-list-plugin.js
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ */
+
+const finder = require('find-package-json');
+const fs = require('fs');
+const path = require('path');
+const validateOptions = require('schema-utils');
+
+/**
+ * The name of this plugin.
+ *
+ * @type {string}
+ */
+const PLUGIN_NAME = 'dependency-list-plugin';
+
+/**
+ * The schema of the configuration options object accepted by the constructor
+ * of DependencyListPlugin.
+ *
+ * @see https://github.com/webpack/schema-utils/blob/v1.0.0/README.md#usage
+ */
+const PLUGIN_OPTIONS_SCHEMA = {
+ type: 'object',
+ properties: {
+
+ /**
+ * The name of the file that should contain the dependency list. By
+ * default, this will be "npm-dependencies.txt".
+ */
+ filename: { type: 'string' },
+
+ /**
+ * The path in which the dependency list file should be saved. By
+ * default, this will be the output path of the Webpack compiler.
+ */
+ path: { type: 'string' }
+
+ },
+ additionalProperties: false
+};
+
+/**
+ * Webpack plugin that automatically lists each of the NPM dependencies
+ * included within any bundles produced by the compile process.
+ */
+class DependencyListPlugin {
+
+ /**
+ * Creates a new DependencyListPlugin configured with the given options.
+ * The options given must conform to the options schema.
+ *
+ * @see PLUGIN_OPTIONS_SCHEMA
+ *
+ * @param {*} options
+ * The configuration options to apply to the plugin.
+ */
+ constructor(options = {}) {
+ validateOptions(PLUGIN_OPTIONS_SCHEMA, options, 'DependencyListPlugin');
+ this.options = options;
+ }
+
+ /**
+ * Entrypoint for all Webpack plugins. This function will be invoked when
+ * the plugin is being associated with the compile process.
+ *
+ * @param {Compiler} compiler
+ * A reference to the Webpack compiler.
+ */
+ apply(compiler) {
+
+ /**
+ * Logger for this plugin.
+ *
+ * @type {Logger}
+ */
+ const logger = compiler.getInfrastructureLogger(PLUGIN_NAME);
+
+ /**
+ * The full path to the output file that should contain the list of
+ * discovered NPM module dependencies.
+ *
+ * @type {string}
+ */
+ const outputFile = path.join(
+ this.options.path || compiler.options.output.path,
+ this.options.filename || 'npm-dependencies.txt'
+ );
+
+ // Wait for compilation to fully complete
+ compiler.hooks.done.tap(PLUGIN_NAME, (stats) => {
+
+ const moduleCoords = {};
+
+ // Map each file used within any bundle built by the compiler to
+ // its corresponding NPM package, ignoring files that have no such
+ // package
+ stats.compilation.fileDependencies.forEach(file => {
+
+ // Locate NPM package corresponding to file dependency (there
+ // may not be one)
+ const moduleFinder = finder(file)
Review comment:
Fixed via rebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#discussion_r644459949
##########
File path: guacamole/src/main/frontend/plugins/dependency-list-plugin.js
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ */
+
+const finder = require('find-package-json');
+const fs = require('fs');
+const path = require('path');
+const validateOptions = require('schema-utils');
+
+/**
+ * The name of this plugin.
+ *
+ * @type {string}
+ */
+const PLUGIN_NAME = 'dependency-list-plugin';
+
+/**
+ * The schema of the configuration options object accepted by the constructor
+ * of DependencyListPlugin.
+ *
+ * @see https://github.com/webpack/schema-utils/blob/v1.0.0/README.md#usage
+ */
+const PLUGIN_OPTIONS_SCHEMA = {
+ type: 'object',
+ properties: {
+
+ /**
+ * The name of the file that should contain the dependency list. By
+ * default, this will be "npm-dependencies.txt".
+ */
+ filename: { type: 'string' },
+
+ /**
+ * The path in which the dependency list file should be saved. By
+ * default, this will be the output path of the Webpack compiler.
+ */
+ path: { type: 'string' }
+
+ },
+ additionalProperties: false
+};
+
+/**
+ * Webpack plugin that automatically lists each of the NPM dependencies
+ * included within any bundles produced by the compile process.
+ */
+class DependencyListPlugin {
+
+ /**
+ * Creates a new DependencyListPlugin configured with the given options.
+ * The options given must conform to the options schema.
+ *
+ * @see PLUGIN_OPTIONS_SCHEMA
+ *
+ * @param {*} options
+ * The configuration options to apply to the plugin.
+ */
+ constructor(options = {}) {
+ validateOptions(PLUGIN_OPTIONS_SCHEMA, options, 'DependencyListPlugin');
+ this.options = options;
+ }
+
+ /**
+ * Entrypoint for all Webpack plugins. This function will be invoked when
+ * the plugin is being associated with the compile process.
+ *
+ * @param {Compiler} compiler
+ * A reference to the Webpack compiler.
+ */
+ apply(compiler) {
+
+ /**
+ * Logger for this plugin.
+ *
+ * @type {Logger}
+ */
+ const logger = compiler.getInfrastructureLogger(PLUGIN_NAME);
+
+ /**
+ * The full path to the output file that should contain the list of
+ * discovered NPM module dependencies.
+ *
+ * @type {string}
+ */
+ const outputFile = path.join(
+ this.options.path || compiler.options.output.path,
+ this.options.filename || 'npm-dependencies.txt'
+ );
+
+ // Wait for compilation to fully complete
+ compiler.hooks.done.tap(PLUGIN_NAME, (stats) => {
+
+ const moduleCoords = {};
+
+ // Map each file used within any bundle built by the compiler to
+ // its corresponding NPM package, ignoring files that have no such
+ // package
+ stats.compilation.fileDependencies.forEach(file => {
+
+ // Locate NPM package corresponding to file dependency (there
+ // may not be one)
+ const moduleFinder = finder(file)
Review comment:
🙀 Oops
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a change in pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on a change in pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#discussion_r644458654
##########
File path: guacamole/src/main/frontend/src/app/index/indexModule.js
##########
@@ -17,22 +17,37 @@
* under the License.
*/
+require('angular-module-shim.js');
Review comment:
What are these imports doing up at the top here? I'm guessing these are just global runtime requirements?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a change in pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on a change in pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#discussion_r644458393
##########
File path: guacamole/src/main/frontend/src/app/index/indexModule.js
##########
@@ -17,22 +17,37 @@
* under the License.
*/
+require('angular-module-shim.js');
+require('relocateParameters.js');
+
+require('angular-translate-interpolation-messageformat');
+require('angular-translate-loader-static-files');
+
/**
* The module for the root of the application.
*/
angular.module('index', [
+
+ require('angular-route'),
+ require('angular-translate'),
+
'auth',
'client',
'clipboard',
'home',
'login',
'manage',
'navigation',
- 'ngRoute',
- 'ngTouch',
'notification',
- 'pascalprecht.translate',
'rest',
'settings',
+
Review comment:
Why is there a space before this last one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#issuecomment-853539164
Well, I think this all looks pretty reasonable to me. Let's do it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner merged pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
jmuehlner merged pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#issuecomment-853426249
The issue with the pagination directive was due to a change in AngularJS 1.8.x and jQuery 3.5.0+ where the automatic XHTML -> HTML filtering has been removed. A self-closing tag like `<div/>` previously would have been translated to `<div></div>`, but now is instead given to the browser verbatim. The browser then interprets it as an unclosed `<div>`, leading to unexpected nesting of elements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#discussion_r644451733
##########
File path: guacamole/src/main/frontend/package.json
##########
@@ -0,0 +1,35 @@
+{
+ "private": true,
+ "scripts": {
+ "build": "webpack --progress"
+ },
+ "dependencies": {
+ "@simonwep/pickr": "1.8.1",
+ "angular": "1.8.2",
+ "angular-route": "1.8.2",
+ "angular-templatecache-webpack-plugin": "^1.0.1",
Review comment:
Reading up on this a bit, it seems the best approach is to use `^` except where there's a specific reason not to:
* It's the default for npm and therefore the least surprising to anyone already well-steeped in that ecosystem.
* It avoids duplicating the duty of `package-lock.json`, which we are including in version control here following the _other_ established best practice for npm.
I definitely prefer the Maven approach of absolute reproducibility by default (with the Maven equivalent to `^` and `~` being considered bad practice and almost never used), but we're stuck with the npm approach here.
I'll update to 🥕s everywhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a change in pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on a change in pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#discussion_r644447944
##########
File path: guacamole/src/main/frontend/package.json
##########
@@ -0,0 +1,35 @@
+{
+ "private": true,
+ "scripts": {
+ "build": "webpack --progress"
+ },
+ "dependencies": {
+ "@simonwep/pickr": "1.8.1",
+ "angular": "1.8.2",
+ "angular-route": "1.8.2",
+ "angular-templatecache-webpack-plugin": "^1.0.1",
Review comment:
Do we plan on having any policy on whether these dependencies should use the `^` operator or not? The `package-lock.json` ensures that builds will be repeatable, but I usually like to lock down the packages to specific versions by removing the `^` and other operators out of `package.json` as well.
I'm not really sure what the right call is here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a change in pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on a change in pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#discussion_r644447944
##########
File path: guacamole/src/main/frontend/package.json
##########
@@ -0,0 +1,35 @@
+{
+ "private": true,
+ "scripts": {
+ "build": "webpack --progress"
+ },
+ "dependencies": {
+ "@simonwep/pickr": "1.8.1",
+ "angular": "1.8.2",
+ "angular-route": "1.8.2",
+ "angular-templatecache-webpack-plugin": "^1.0.1",
Review comment:
Do we plan on having any policy on whether these dependencies should use the `^` operator or not? The `package-lock.json` ensures that builds will be repeatable, but I usually like to lock down the packages to specific versions by removing the `^` and other operators out of `package.json` as well.
I'm not really sure what the right call is here.
##########
File path: guacamole/src/main/frontend/src/app/index/indexModule.js
##########
@@ -17,22 +17,37 @@
* under the License.
*/
+require('angular-module-shim.js');
+require('relocateParameters.js');
+
+require('angular-translate-interpolation-messageformat');
+require('angular-translate-loader-static-files');
+
/**
* The module for the root of the application.
*/
angular.module('index', [
+
+ require('angular-route'),
+ require('angular-translate'),
+
'auth',
'client',
'clipboard',
'home',
'login',
'manage',
'navigation',
- 'ngRoute',
- 'ngTouch',
'notification',
- 'pascalprecht.translate',
'rest',
'settings',
+
Review comment:
Why is there a space before this last one?
##########
File path: guacamole/src/main/frontend/src/app/index/indexModule.js
##########
@@ -17,22 +17,37 @@
* under the License.
*/
+require('angular-module-shim.js');
Review comment:
What are these imports doing up at the top here? I'm guessing these are just global runtime requirements?
##########
File path: guacamole/src/main/frontend/src/app/index/indexModule.js
##########
@@ -17,22 +17,37 @@
* under the License.
*/
+require('angular-module-shim.js');
+require('relocateParameters.js');
+
+require('angular-translate-interpolation-messageformat');
+require('angular-translate-loader-static-files');
+
/**
* The module for the root of the application.
*/
angular.module('index', [
+
+ require('angular-route'),
Review comment:
Why are these here?
##########
File path: guacamole/src/main/frontend/plugins/dependency-list-plugin.js
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ */
+
+const finder = require('find-package-json');
+const fs = require('fs');
+const path = require('path');
+const validateOptions = require('schema-utils');
+
+/**
+ * The name of this plugin.
+ *
+ * @type {string}
+ */
+const PLUGIN_NAME = 'dependency-list-plugin';
+
+/**
+ * The schema of the configuration options object accepted by the constructor
+ * of DependencyListPlugin.
+ *
+ * @see https://github.com/webpack/schema-utils/blob/v1.0.0/README.md#usage
+ */
+const PLUGIN_OPTIONS_SCHEMA = {
+ type: 'object',
+ properties: {
+
+ /**
+ * The name of the file that should contain the dependency list. By
+ * default, this will be "npm-dependencies.txt".
+ */
+ filename: { type: 'string' },
+
+ /**
+ * The path in which the dependency list file should be saved. By
+ * default, this will be the output path of the Webpack compiler.
+ */
+ path: { type: 'string' }
+
+ },
+ additionalProperties: false
+};
+
+/**
+ * Webpack plugin that automatically lists each of the NPM dependencies
+ * included within any bundles produced by the compile process.
+ */
+class DependencyListPlugin {
+
+ /**
+ * Creates a new DependencyListPlugin configured with the given options.
+ * The options given must conform to the options schema.
+ *
+ * @see PLUGIN_OPTIONS_SCHEMA
+ *
+ * @param {*} options
+ * The configuration options to apply to the plugin.
+ */
+ constructor(options = {}) {
+ validateOptions(PLUGIN_OPTIONS_SCHEMA, options, 'DependencyListPlugin');
+ this.options = options;
+ }
+
+ /**
+ * Entrypoint for all Webpack plugins. This function will be invoked when
+ * the plugin is being associated with the compile process.
+ *
+ * @param {Compiler} compiler
+ * A reference to the Webpack compiler.
+ */
+ apply(compiler) {
+
+ /**
+ * Logger for this plugin.
+ *
+ * @type {Logger}
+ */
+ const logger = compiler.getInfrastructureLogger(PLUGIN_NAME);
+
+ /**
+ * The full path to the output file that should contain the list of
+ * discovered NPM module dependencies.
+ *
+ * @type {string}
+ */
+ const outputFile = path.join(
+ this.options.path || compiler.options.output.path,
+ this.options.filename || 'npm-dependencies.txt'
+ );
+
+ // Wait for compilation to fully complete
+ compiler.hooks.done.tap(PLUGIN_NAME, (stats) => {
+
+ const moduleCoords = {};
+
+ // Map each file used within any bundle built by the compiler to
+ // its corresponding NPM package, ignoring files that have no such
+ // package
+ stats.compilation.fileDependencies.forEach(file => {
+
+ // Locate NPM package corresponding to file dependency (there
+ // may not be one)
+ const moduleFinder = finder(file)
Review comment:
No semicolon?
##########
File path: guacamole/src/main/frontend/src/app/index/indexModule.js
##########
@@ -17,22 +17,37 @@
* under the License.
*/
+require('angular-module-shim.js');
+require('relocateParameters.js');
+
+require('angular-translate-interpolation-messageformat');
+require('angular-translate-loader-static-files');
+
/**
* The module for the root of the application.
*/
angular.module('index', [
+
+ require('angular-route'),
+ require('angular-translate'),
+
'auth',
'client',
'clipboard',
'home',
'login',
'manage',
'navigation',
- 'ngRoute',
- 'ngTouch',
'notification',
- 'pascalprecht.translate',
'rest',
'settings',
+
Review comment:
Makes sense to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a change in pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on a change in pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#discussion_r644459339
##########
File path: guacamole/src/main/frontend/plugins/dependency-list-plugin.js
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ */
+
+const finder = require('find-package-json');
+const fs = require('fs');
+const path = require('path');
+const validateOptions = require('schema-utils');
+
+/**
+ * The name of this plugin.
+ *
+ * @type {string}
+ */
+const PLUGIN_NAME = 'dependency-list-plugin';
+
+/**
+ * The schema of the configuration options object accepted by the constructor
+ * of DependencyListPlugin.
+ *
+ * @see https://github.com/webpack/schema-utils/blob/v1.0.0/README.md#usage
+ */
+const PLUGIN_OPTIONS_SCHEMA = {
+ type: 'object',
+ properties: {
+
+ /**
+ * The name of the file that should contain the dependency list. By
+ * default, this will be "npm-dependencies.txt".
+ */
+ filename: { type: 'string' },
+
+ /**
+ * The path in which the dependency list file should be saved. By
+ * default, this will be the output path of the Webpack compiler.
+ */
+ path: { type: 'string' }
+
+ },
+ additionalProperties: false
+};
+
+/**
+ * Webpack plugin that automatically lists each of the NPM dependencies
+ * included within any bundles produced by the compile process.
+ */
+class DependencyListPlugin {
+
+ /**
+ * Creates a new DependencyListPlugin configured with the given options.
+ * The options given must conform to the options schema.
+ *
+ * @see PLUGIN_OPTIONS_SCHEMA
+ *
+ * @param {*} options
+ * The configuration options to apply to the plugin.
+ */
+ constructor(options = {}) {
+ validateOptions(PLUGIN_OPTIONS_SCHEMA, options, 'DependencyListPlugin');
+ this.options = options;
+ }
+
+ /**
+ * Entrypoint for all Webpack plugins. This function will be invoked when
+ * the plugin is being associated with the compile process.
+ *
+ * @param {Compiler} compiler
+ * A reference to the Webpack compiler.
+ */
+ apply(compiler) {
+
+ /**
+ * Logger for this plugin.
+ *
+ * @type {Logger}
+ */
+ const logger = compiler.getInfrastructureLogger(PLUGIN_NAME);
+
+ /**
+ * The full path to the output file that should contain the list of
+ * discovered NPM module dependencies.
+ *
+ * @type {string}
+ */
+ const outputFile = path.join(
+ this.options.path || compiler.options.output.path,
+ this.options.filename || 'npm-dependencies.txt'
+ );
+
+ // Wait for compilation to fully complete
+ compiler.hooks.done.tap(PLUGIN_NAME, (stats) => {
+
+ const moduleCoords = {};
+
+ // Map each file used within any bundle built by the compiler to
+ // its corresponding NPM package, ignoring files that have no such
+ // package
+ stats.compilation.fileDependencies.forEach(file => {
+
+ // Locate NPM package corresponding to file dependency (there
+ // may not be one)
+ const moduleFinder = finder(file)
Review comment:
No semicolon?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#issuecomment-853409310
So far, I'm seeing the following regressions:
* The pagination directive changed from this:
to this:
* The "Pickr" color picker is no longer detected as unavailable on IE10 and IE11. The service handling initialization of the Pickr library should be recognizing that things will not work and mark Pickr as unavailable, but this isn't happening.
We should also review and check against the 1.6.x to 1.7.x migration guide for AngularJS:
https://docs.angularjs.org/guide/migration#migrating-from-1-6-to-1-7
There are no applicable breaking changes for 1.7.x to 1.8.x, which affected only jqLite (we use full jQuery instead of this).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#discussion_r644459273
##########
File path: guacamole/src/main/frontend/src/app/index/indexModule.js
##########
@@ -17,22 +17,37 @@
* under the License.
*/
+require('angular-module-shim.js');
Review comment:
Yep, exactly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#issuecomment-853409310
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#discussion_r644459209
##########
File path: guacamole/src/main/frontend/src/app/index/indexModule.js
##########
@@ -17,22 +17,37 @@
* under the License.
*/
+require('angular-module-shim.js');
+require('relocateParameters.js');
+
+require('angular-translate-interpolation-messageformat');
+require('angular-translate-loader-static-files');
+
/**
* The module for the root of the application.
*/
angular.module('index', [
+
+ require('angular-route'),
+ require('angular-translate'),
+
'auth',
'client',
'clipboard',
'home',
'login',
'manage',
'navigation',
- 'ngRoute',
- 'ngTouch',
'notification',
- 'pascalprecht.translate',
'rest',
'settings',
+
Review comment:
I thought it made sense to visually separate things into three groups:
* AngularJS modules from external libraries
* AngularJS modules we define ourselves with actual code
* AngularJS modules that are generated at build time (just `templates-main`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#issuecomment-853432195
The issue with Pickr and IE10/IE11 was due to `Pickr.create()` no longer throwing an exception outright if browser support is missing. Instead, we must rely on the lack of an "init" event from the created Pickr object to detect whether Pickr can be used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #616: GUACAMOLE-773: Update all JavaScript dependencies to latest versions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #616:
URL: https://github.com/apache/guacamole-client/pull/616#discussion_r644460262
##########
File path: guacamole/src/main/frontend/plugins/dependency-list-plugin.js
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ */
+
+const finder = require('find-package-json');
+const fs = require('fs');
+const path = require('path');
+const validateOptions = require('schema-utils');
+
+/**
+ * The name of this plugin.
+ *
+ * @type {string}
+ */
+const PLUGIN_NAME = 'dependency-list-plugin';
+
+/**
+ * The schema of the configuration options object accepted by the constructor
+ * of DependencyListPlugin.
+ *
+ * @see https://github.com/webpack/schema-utils/blob/v1.0.0/README.md#usage
+ */
+const PLUGIN_OPTIONS_SCHEMA = {
+ type: 'object',
+ properties: {
+
+ /**
+ * The name of the file that should contain the dependency list. By
+ * default, this will be "npm-dependencies.txt".
+ */
+ filename: { type: 'string' },
+
+ /**
+ * The path in which the dependency list file should be saved. By
+ * default, this will be the output path of the Webpack compiler.
+ */
+ path: { type: 'string' }
+
+ },
+ additionalProperties: false
+};
+
+/**
+ * Webpack plugin that automatically lists each of the NPM dependencies
+ * included within any bundles produced by the compile process.
+ */
+class DependencyListPlugin {
+
+ /**
+ * Creates a new DependencyListPlugin configured with the given options.
+ * The options given must conform to the options schema.
+ *
+ * @see PLUGIN_OPTIONS_SCHEMA
+ *
+ * @param {*} options
+ * The configuration options to apply to the plugin.
+ */
+ constructor(options = {}) {
+ validateOptions(PLUGIN_OPTIONS_SCHEMA, options, 'DependencyListPlugin');
+ this.options = options;
+ }
+
+ /**
+ * Entrypoint for all Webpack plugins. This function will be invoked when
+ * the plugin is being associated with the compile process.
+ *
+ * @param {Compiler} compiler
+ * A reference to the Webpack compiler.
+ */
+ apply(compiler) {
+
+ /**
+ * Logger for this plugin.
+ *
+ * @type {Logger}
+ */
+ const logger = compiler.getInfrastructureLogger(PLUGIN_NAME);
+
+ /**
+ * The full path to the output file that should contain the list of
+ * discovered NPM module dependencies.
+ *
+ * @type {string}
+ */
+ const outputFile = path.join(
+ this.options.path || compiler.options.output.path,
+ this.options.filename || 'npm-dependencies.txt'
+ );
+
+ // Wait for compilation to fully complete
+ compiler.hooks.done.tap(PLUGIN_NAME, (stats) => {
+
+ const moduleCoords = {};
+
+ // Map each file used within any bundle built by the compiler to
+ // its corresponding NPM package, ignoring files that have no such
+ // package
+ stats.compilation.fileDependencies.forEach(file => {
+
+ // Locate NPM package corresponding to file dependency (there
+ // may not be one)
+ const moduleFinder = finder(file)
Review comment:
Fixed via rebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org