chore: ability to add external libs (axios) #990

jeeyyy · 2018-07-03T23:51:36Z

The aim of this PR is to allow for adding of external libraries that are readily available and well tested, to avoid overhead of creating self contained utilities within axe (not re-invent the wheel).
The implementation shows adding external library axios (which allows handling network requests), with some generic code written for allowing easier addition of newer libraries in the future.
It is achieved by modification to the grunt build process. A new task generate-imports is added as a sub-task with in build step. This prepares the external assets by mounting them on axe.imports object as is (if not UMD wrapped), or after unwrapping UMD resolver and getting the factory to be mounted.
axios was chosen over superagent based on this perf test - https://jsperf.com/axios-vs-superagent/

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

Follows the commit message policy, appropriate for next version
Has documentation updated, a DU ticket, or requires no documentation change
Includes new tests, or was unnecessary
Code is reviewed for security by: @WilcoFiers

WilcoFiers · 2018-07-04T07:40:09Z

None of these are things that impact the users, so "feat" and "fix" are inappropriate for this PR.

WilcoFiers · 2018-07-04T07:55:06Z

Also please don't fill out the checkboxes they are "to be filled out by PR reviewer(s)"

jeeyyy · 2018-07-04T10:33:02Z

@WilcoFiers - In it's true sense this is a new feature, albeit unseen by users. We are creating a feature of being able to add external dependencies. What prefix/ commit standard works better? chore?

WilcoFiers

I think you should add a test to this to ensure axios is included in axe.js.

jeeyyy · 2018-07-05T08:11:41Z

@WilcoFiers - I have added the test for axios. Please squash & merge, so the commit can be prefixed as chore. Thank you.

Changes Done. Please review again.

WilcoFiers · 2018-07-05T19:50:53Z

Recording what we discussed earlier today:

Axios will have to be put on axe.import or something so that it can't be overwritten
Axios will have to be exposed in axe.source as well so it's still around when we inject axe.

jeeyyy · 2018-07-06T06:49:16Z

Have hit a roadblock with this one now.

Attempting to inject an external library into axe.imports namespace, creates side effects like overriding define[] & module.exports from axe. This is seen only when using axe via require as against script inlining which works perfectly.

Mounting the library on axe.imports was done by reading the lib & writing to file, with in axe/core/imports directory, which was later picked up by the build step to concat.

return `
/* global axe, axios */
axe.imports["${library}"] = (function() {
let externalExistence = window["${library}"];
${grunt.file.read(source)} 

if(window["${library}"]) {
	delete window["${library}"];
}
if (externalExistence) {
	window["${library}"] = externalExistence
}
return ${library};
})();
`;

At this juncture, believe there are only a few ways of moving forward with this.

Use the current implementation in this PR, wherein we expose dependencies as globals.
Embrace requirejs for managing dependency and bundling (build step changes).
Introduce progressive typescript, the tsc - compiler as a pre-step to build, which then allows doing import statements in ts files.

Alternative suggestions welcome.

stephenmathieson · 2018-07-06T14:30:36Z

@JKODU I think I have a pretty simple solution to the define() and module.exports checks. We can just create a shadowing (scoped) variables tricking Axios' UMD wrapper. For example:

axe.imports.axios = (function () {
  // Clobber these globals, tricking UMD
  var define = undefined
  var module = undefined
  var exports = undefined

  // Keep a copy of the previous Axios global (if set)
  var _axios = window.axios

  ${axios_source_code}

  var axios = window.axios

  // Reset window.axios (if necessary)
  delete window.axios
  if (_axios) {
    window.axios = _axios
  }

  // Return "our" copy of Axios
  return axios
})()

jeeyyy · 2018-07-09T21:31:29Z

@WilcoFiers - review again

WilcoFiers · 2018-07-10T11:43:09Z

.eslintignore

@@ -0,0 +1,3 @@
+build/tasks/generate-imports.js
+!lib/core/imports/index.js
+lib/core/imports/*.js


You'll need to add this to gitignore as well.

WilcoFiers · 2018-07-10T11:46:47Z

test/integration/full/umd/umd-window.js

+	});
+
+	// @wilco - need your input on this, there is no axe.source or equivalent available
+	// it('should ensure axios source includes axios', function() {


axe.source will be available is module.exports is defined. This test should work in umd-defined.js.

WilcoFiers · 2018-07-10T11:47:43Z

build/tasks/generate-imports.js

+			// list of external dependencies,
+			// which needs to be added to axe.imports object
+			const LIBS_TO_IMPORT = {
+				axios: './node_modules/axios/dist/axios.js'


This should come from the gruntfile.js rather than live in the task.

WilcoFiers · 2018-07-10T11:53:04Z

build/tasks/generate-imports.js

+				processImport(key, LIBS_TO_IMPORT[key]);
+			});
+
+			done();


Looks like this function is sync. why did you need this.async()?

You are right, initially I was using fs with promises for file read write, but got rid of it. This can go.

WilcoFiers · 2018-07-10T11:54:56Z

build/tasks/generate-imports.js

+/*eslint-env node */
+const UglifyJS = require('uglify-js');
+
+module.exports = grunt => {


This is going to be difficult to maintain without tests preventing regression.

There are tests validating axios in umd integration tests. As we add multiple libraries, we can enhance the tests around each library.

WilcoFiers · 2018-07-10T11:59:37Z

build/tasks/generate-imports.js

+				return (
+					/typeof exports/.test(sourceCode) &&
+					/typeof define/ &&
+					/typeof module/


Did you mean to put .test() on these?

jeeyyy · 2018-07-10T15:02:16Z

@WilcoFiers

updated umd tests
good catch on //.test missing, added
migrated library(s) to import to come from Gruntfile.
tested generated axe on node, browser, on axe extensions (Worldspace) with custom rule to verify axe.imports.

Please review again.

stephenmathieson · 2018-07-10T17:01:19Z

build/tasks/generate-imports.js

+		'Task for generating an axe.imports module with external dependencies.',
+		function() {
+			// Convenience method that utilises uglifyjs tree-transformer to unwrap umd module resolver
+			const removeUMD = new UglifyJS.TreeTransformer(node => {


We should consider moving this to something like Babel (in the long-term). No need to change now.

stephenmathieson · 2018-07-10T17:02:55Z

build/tasks/generate-imports.js

+				}
+			};
+
+			const hasUmdWrapper = sourceCode => {


This is a little scary and seems brittle. I think it's fine for now, since we know Axios has a UMD wrapper, but before we start adding more "imports", we may want to revisit this.

stephenmathieson · 2018-07-10T17:04:06Z

build/tasks/generate-imports.js

+			};
+
+			const writeLibrary = (libName, factory) => {
+				const lib = `/* global axe */ axe.imports["${libName}"] = ${factory}`;


No need for this ESLint directive, since we're ignoring this file anyway 😉

stephenmathieson · 2018-07-10T17:05:03Z

lib/core/imports/index.js

@@ -0,0 +1,10 @@
+/*eslint no-unused-vars: 0*/
+/* exported utils */


What is this directive for?

stephenmathieson · 2018-07-10T17:05:19Z

lib/core/imports/index.js

+ * @memberof axe
+ */
+
+var imports = (axe.imports = {});


I don't think we need var imports here. Can probably just have axe.imports = {}.

stephenmathieson

LGTM! Great work here dude 😄

WilcoFiers

I've tested this in both Attest-node and the Attest extension to make sure we don't lose Axios somewhere while injecting it. Works like a charm!

jeeyyy added 2 commits July 3, 2018 23:55

fix: enhance grunt to concat external deps

8c4e7eb

fix: refactor concatenation to avoid global overrides

770a386

jeeyyy requested a review from WilcoFiers as a code owner July 3, 2018 23:51

test: refine umd test to allow multiple global in same file

d3137c0

jeeyyy mentioned this pull request Jul 4, 2018

feat(preload): cssom assets #958

Merged

4 tasks

jeeyyy changed the title ~~feat: ability to add external libs~~ chore: ability to add external libs Jul 4, 2018

jeeyyy requested review from dylanb, stephenmathieson and isner July 4, 2018 12:45

jeeyyy changed the title ~~chore: ability to add external libs~~ chore: ability to add external libs (axios) Jul 4, 2018

Merge branch 'develop' into add-external-libs

55dbe8b

WilcoFiers previously requested changes Jul 4, 2018

View reviewed changes

jeeyyy added 2 commits July 5, 2018 10:08

fix: merge from develop

baf804d

test: verify axios is exposed as a global

0c5be83

jeeyyy changed the title ~~chore: ability to add external libs (axios)~~ [WIP] chore: ability to add external libs (axios) Jul 5, 2018

jeeyyy added info needed More information or research is needed to continue needs discussion More discussion is needed to continue labels Jul 6, 2018

chore: attempt new way to import externals

c2fe01b

jeeyyy removed needs discussion More discussion is needed to continue info needed More information or research is needed to continue labels Jul 8, 2018

jeeyyy added 2 commits July 9, 2018 21:29

chore: add axios library to axe imports

6e45ec0

chore: refactor & add generating imports as a build task

d18d42b

jeeyyy changed the title ~~[WIP] chore: ability to add external libs (axios)~~ chore: ability to add external libs (axios) Jul 9, 2018

chore: udpate grunt task with done cb to maintain sync.

216a2fc

jeeyyy mentioned this pull request Jul 9, 2018

[CLOSING > See: #1081] feat(rule): css-orientation-lock new wcag21 rule #971

Closed

4 tasks

chore: understand args length passed to umd wrapper

629d0cd

WilcoFiers reviewed Jul 10, 2018

View reviewed changes

jeeyyy added 3 commits July 10, 2018 15:21

test: added axios source check in axe source.

1905cbd

fix: updates based on review comments.

18afbf4

fix: pass libs to import from gruntfile

e7bb4a2

stephenmathieson reviewed Jul 10, 2018

View reviewed changes

fix: updates based on review.

38b9e85

stephenmathieson approved these changes Jul 10, 2018

View reviewed changes

WilcoFiers approved these changes Jul 12, 2018

View reviewed changes

jeeyyy merged commit 0957dab into develop Jul 12, 2018

jeeyyy deleted the add-external-libs branch July 12, 2018 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: ability to add external libs (axios) #990

chore: ability to add external libs (axios) #990

jeeyyy commented Jul 3, 2018 •

edited by WilcoFiers

Loading

WilcoFiers commented Jul 4, 2018

WilcoFiers commented Jul 4, 2018

jeeyyy commented Jul 4, 2018

WilcoFiers left a comment

jeeyyy commented Jul 5, 2018

WilcoFiers commented Jul 5, 2018

jeeyyy commented Jul 6, 2018

stephenmathieson commented Jul 6, 2018

jeeyyy commented Jul 9, 2018

WilcoFiers Jul 10, 2018

WilcoFiers Jul 10, 2018

WilcoFiers Jul 10, 2018

WilcoFiers Jul 10, 2018

jeeyyy Jul 10, 2018

WilcoFiers Jul 10, 2018

jeeyyy Jul 10, 2018

WilcoFiers Jul 10, 2018

jeeyyy commented Jul 10, 2018

stephenmathieson Jul 10, 2018

stephenmathieson Jul 10, 2018

stephenmathieson Jul 10, 2018

stephenmathieson Jul 10, 2018

stephenmathieson Jul 10, 2018

stephenmathieson left a comment

WilcoFiers left a comment

		@@ -0,0 +1,10 @@
		/eslint no-unused-vars: 0/
		/* exported utils */

chore: ability to add external libs (axios) #990

chore: ability to add external libs (axios) #990

Conversation

jeeyyy commented Jul 3, 2018 • edited by WilcoFiers Loading

Reviewer checks

WilcoFiers commented Jul 4, 2018

WilcoFiers commented Jul 4, 2018

jeeyyy commented Jul 4, 2018

WilcoFiers left a comment

Choose a reason for hiding this comment

jeeyyy commented Jul 5, 2018

WilcoFiers commented Jul 5, 2018

jeeyyy commented Jul 6, 2018

stephenmathieson commented Jul 6, 2018

jeeyyy commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeeyyy commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenmathieson left a comment

Choose a reason for hiding this comment

WilcoFiers left a comment

Choose a reason for hiding this comment

jeeyyy commented Jul 3, 2018 •

edited by WilcoFiers

Loading