chore: using browserify to import dependencies #1256

jeeyyy · 2018-11-22T14:04:44Z

This is a developer workflow enhancement PR.

Ability to require external modules has been difficult in Axe.
Previous attempts to require and inject modules into axe global have been not stable, due to having to adapt for various module bundling standards followed by different dependencies.

This PR, circumvents the above problems and introduces a fluent way to require, by embracing browserify.

Removed old generate-imports grunt multi task, which was flaky.
Introduce npm script - npm run imports-gen which passes dependencies to mount on axe as arguments.
axe.imports = { ...myDependencies } is constructed at build time.
Deleted copy pasted library css-selecter-parser and changed into as a require dependency.

Closes issue: NA

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

stephenmathieson · 2018-11-26T17:28:53Z

Gruntfile.js

@@ -340,6 +329,10 @@ module.exports = function(grunt) {
 			}
 		},
 		run: {
+			npm_run_imports: {


Can we define this in package.json#scripts instead?

{ "scripts": { "build": "grunt build", "prebuild": "npm run imports-gen" } }

This way, we don't get the same confusion we had in #1252/#1250.

I thought about this @stephenmathieson, but this required developer documentation update at various places.

Our docs are tied to steps like grunt dev, grunt build and grunt translate. So the only way to trigger any current npm script is via grunt-run, like above.

You concern of what confusion happened in above #1252 etc., will not re-occur as I have already stripped out multiple references to same task and refactored it.

As you can see from most of the chore PR's, once we more to pure npm scripts, it will be easier to update documentation when we have change all of the grunt tasks. So the answer is NO for the time being.

As if I do this change above now, a developer running grunt build will yield different results to npm run build.

That may be true about grunt-run and so on, so I can understand not wanting to take all of that on. As an aside, though, I wouldn't let developer docs stop you from making any changes; we can always update the docs. I'm happy to help with that!

Some of my chore work in axe-core is moving away from grunt to npm scripts. I will make the switch to above recommendation in a different PR when time is right :).

stephenmathieson · 2018-11-26T17:29:36Z

build/imports-generator.js

@@ -0,0 +1,109 @@
+/* global Promise */


This is not needed. Promise is defined in all versions of Node.js we support.

Unfortunately not, our eslint rules ain't happy for that.

Oh strange. PR incoming :)

This directive can now be removed.

stephenmathieson · 2018-11-26T19:36:55Z

build/imports-generator.js

+function parseArgsAsRequireMap(args) {
+	const argMap = {};
+
+	if (args.length % 2) {


Please add a comment explaining why this validates arguments.

stephenmathieson · 2018-11-26T19:38:11Z

build/imports-generator.js

+ */
+function createFile(path, content) {
+	return new Promise((resolve, reject) => {
+		mkdirp(getDirName(path), err => {


Consider replacing this with make-dir. mkdirp is unmaintained and does not return a Promise.

const {dirname:getDirName} = require('path') const makeDir = require('make-dir') const {writeFile} = require('fs/promises') const createFile = (path, content) => makeDir(getDirName(path)).then(() => writeFile(path, content))

I am having minimal luck with fs/promises for some reason.
Cheers for the make-dir suggestion.

Asides fs/promises API is have a stability level 1 & experimental flag.

stephenmathieson · 2018-11-26T19:44:31Z

package.json

@@ -56,6 +56,7 @@
 	},
 	"scripts": {
 		"api-docs": "jsdoc --configure .jsdoc.json",
+		"imports-gen": "node ./build/imports-generator --axios 'axios' --doT 'dot' --CssSelectorParser 'css-selector-parser'",


I'm concerned that if we keep this mapping as CLI flags, the list may become very difficult to maintain in the future.

Why not just add the mapping to ./build/imports-generator.js?

const mapping = [ ['axios','axios'], ['doT', 'dot'], ['CssSelectorParser', 'css-selector-parser'] ]

Agree, as easy as it is to add a mapping to the imports-generator file. I want this to be reminder that we need to update the code-base to allow for fluent import statements, so the more we add here, we shall prioritise the build system changes.

Happy to move it though. Thoughts?

On second thought moved it to mapping...

stephenmathieson · 2018-11-26T19:46:12Z

build/imports-generator.js

+	}
+
+	// browserify `lib/core/imports/index.js`
+	browserify(importsFile).bundle(async (err, result) => {


Are we sure this is going to work in all cases? What about node.js usage?

Some of our tests ensure that, what gets injected into axe.imports is as expected.

I have done some node testing locally. But we will ensure that it works before a release.

How do you expect we'll "ensure that it works before a release"? Instead of waiting until release-time, can we perform those tests on your branch?

Will discuss with @WilcoFiers.

marcysutton · 2018-11-27T01:12:43Z

build/imports-generator.js

+
+/**
+ * Convert arguments to object map of require statement
+ * @param {Array<String>} args key value pair of arguments


Can you put an example here? "Key value pair" sounds like an object, but it's an array of strings.

marcysutton · 2018-11-27T01:13:39Z

build/imports-generator.js

+		throw new Error('Invalid number of arguments');
+	}
+
+	for (let i = 0; i < args.length; i += 2) {


Comments for this loop would be good too, since it has an atypical increment (i += 2) and some magic replacing going on below.

Updated. Review again.

stephenmathieson · 2018-11-27T16:54:50Z

build/imports-generator.js

+	// browserify `lib/core/imports/index.js`
+	browserify(importsFile).bundle(async (err, result) => {
+		if (err) {
+			throw new Error('Cannot browserify axe.imports', err);


I'm not sure it makes sense to use "browserify" as a verb 🤔

Any thoughts here?

No change here. Leaving as is.

dylanb · 2018-11-30T20:58:34Z

Could @marcysutton and/or @stephenmathieson review to make sure their issues have been addressed and then let me know and I'll review?

stephenmathieson · 2018-11-30T21:07:39Z

@dylanb I'm waiting on a response for #1256 (comment)

WilcoFiers · 2018-11-30T12:47:58Z

.gitignore

 typings/axe-core/axe-core-tests.js
-doc/rule-descriptions.*.md


Why did you remove this?

WilcoFiers · 2018-11-30T12:50:42Z

Gruntfile.js

+			npm_run_eslint: {
+				cmd: 'npm',
+				args: ['run', 'eslint']
+			},


These ESLint changes should probably be in their own PR.

WilcoFiers · 2018-11-30T12:53:13Z

build/imports-generator.js

+const moduleMap = {
+	axios: 'axios',
+	doT: 'dot',
+	CssSelectorParser: 'css-selector-parser'


I'd prefer this be in a separate PR too.

Sure, but I need this PR to land first then.

WilcoFiers · 2018-12-02T17:54:33Z

build/imports-generator.js

+
+async function run() {
+	// generate `axe.imports` content
+	const fileContent = `


I'm not in favour of dynamically generating this file. Let's just put this file in source control and have this import-generator copy over into tmp.

Updated.

jeeyyy · 2018-12-04T10:44:36Z

@dylanb - thanks for the comment. I believe @WilcoFiers has a handle on this for review and approvals.

WilcoFiers · 2018-12-04T19:50:46Z

Looks fine to me. @JKODU can you run this in the Attest extension and in Attest node to make sure this doesn't accidentally break anything?

jeeyyy · 2018-12-06T15:43:46Z

@WilcoFiers

Tested both extensions and node suite. Works 👏

stephenmathieson · 2018-12-06T17:54:18Z

build/imports-generator.js

@@ -0,0 +1,109 @@
+/* global Promise */


This directive can now be removed.

stephenmathieson · 2018-12-06T17:54:36Z

build/imports-generator.js

+	// browserify `lib/core/imports/index.js`
+	browserify(importsFile).bundle(async (err, result) => {
+		if (err) {
+			throw new Error('Cannot browserify axe.imports', err);


Any thoughts here?

stephenmathieson · 2018-12-06T17:57:09Z

build/imports-generator.js

+ * @param {String} content contents of the file
+ * @returns {Promise}
+ */
+function createFile(path, content) {


Not a blocker, but I'd still like to see this simplified. Maybe:

const makeDir = require('make-dir') const fs = require('fs') const {promisify} = require('util') const writeFile = promisify(fs.writeFile) const createFile = (path, content) => makeDir(getDirName(path)).then(() => writeFile(path, content))

Between, this PR and the axe rule generator PR, there is more or less the same createFile fn. Once both are merged. I plan to refactor this into a shared utility fn.

chore: using browserify to import dependencies

2284c94

jeeyyy requested a review from a team as a code owner November 22, 2018 14:04

WilcoFiers requested a review from stephenmathieson November 26, 2018 16:25

stephenmathieson reviewed Nov 26, 2018

View reviewed changes

marcysutton previously requested changes Nov 27, 2018

View reviewed changes

jeeyyy added 4 commits November 27, 2018 14:34

Merge branch 'develop' into browserify-imports

77f0b36

refactor: updates based on code review

47280c6

fix: re-add grunt-run

b16273a

fix: update npm script to remove args for imports gen task

b04b414

Merge branch 'develop' into browserify-imports

6ac85ef

jeeyyy requested review from stephenmathieson and marcysutton November 27, 2018 15:11

stephenmathieson reviewed Nov 27, 2018

View reviewed changes

jeeyyy added 3 commits November 27, 2018 22:47

Merge branch 'develop' into browserify-imports

30bee4e

Merge branch 'develop' into browserify-imports

41a2ecf

Merge branch 'develop' into browserify-imports

9b8e6fd

WilcoFiers previously requested changes Dec 2, 2018

View reviewed changes

fix: simplify browserify imports

0957856

jeeyyy requested a review from WilcoFiers December 4, 2018 10:43

docs: add jsdoc comments for axe.imports

59808b7

jeeyyy mentioned this pull request Dec 4, 2018

chore: run eslint as a part of grunt dev #1267

Merged

4 tasks

Merge branch 'develop' into browserify-imports

187ef52

stephenmathieson reviewed Dec 6, 2018

View reviewed changes

jeeyyy added 2 commits December 11, 2018 15:42

Merge branch 'develop' into browserify-imports

e939673

refactor: based on code review

1f233d2

WilcoFiers approved these changes Dec 17, 2018

View reviewed changes

Merge branch 'develop' into browserify-imports

38b9b3f

jeeyyy merged commit eed6589 into develop Dec 17, 2018

jeeyyy deleted the browserify-imports branch December 17, 2018 14:19

		typings/axe-core/axe-core-tests.js
		doc/rule-descriptions.*.md

chore: using browserify to import dependencies #1256

chore: using browserify to import dependencies #1256

Conversation

jeeyyy commented Nov 22, 2018 • edited by WilcoFiers Loading

Reviewer checks

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 Nov 27, 2018 • edited Loading

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

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

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

dylanb commented Nov 30, 2018

stephenmathieson commented Nov 30, 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

jeeyyy commented Dec 4, 2018

WilcoFiers commented Dec 4, 2018

jeeyyy commented Dec 6, 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

jeeyyy commented Nov 22, 2018 •

edited by WilcoFiers

Loading

jeeyyy Nov 27, 2018 •

edited

Loading