Closed patrickarlt closed 7 years ago
Thank you for taking the time to improve ally.js!
As far as I can see the esm
modules only cover ally.js itself, but not the imported node_modules array.prototype.findindex
, css.escape
and platform
. These would still resolve to their respective UMD module. Isn't that a problem?
I see you've added a .babelrc
file for the esm
build. Is that because the configuration { "modules": false }
can't be provided via CLI?
Yup the .babelrc
file is specifically for turning off modules. I couldn't another way to do it.
The array.prototype.findindex
, css.escape
and platform
dependencies don't seem to be causing me much of a problem for my specific use case. Looking at the Angular Ahead-of-Time compiler doc again I think the restriction around only using ES 2015 modules only applies to your immediate application code so I can't call require
in my app code but something I import from node_modules
can. Webpack is probably then smoothing over the difference once the Angular compiler has done its work.
Let me do a little more digging to make sure this works out of the box for webpack 2 and rollup users not just for my specific use case.
Ok I've done a little more digging around how this treats dependencies. The original issues that I was trying to solve with this PR was when using the Angular 2 AoT compiler the raw ES 2015 source would get pulled into the bundle breaking both minification (Uglify JS) and older browsers that did support some parts of the syntax (like IE 11, ect...). Adding this new build target of compiled ES 2015 code but retaining the ES 2015 module syntax fixed both issues.
Inspecting the output of my application with source-map-explorer
and grepping the outputs the dependencies on array.prototype.findindex
, css.escape
and platform
appears to be included in my resulting bundle and working fine.
To make sure this worked in a more standard use case (Rollup and webpack 2 with Babel) I made the following app:
// modules are in dist because I have this linked locally. Would be ally.js/esm once released
import whenKey from 'ally.js/dist/esm/when/key.js';
import whenVisableArea from 'ally.js/dist/esm/when/visible-area.js';
import maintainHidden from 'ally.js/dist/esm/maintain/hidden.js';
import maintainDisabled from 'ally.js/dist/esm/maintain/disabled.js';
import maintainTabFocus from 'ally.js/dist/esm/maintain/tab-focus.js';
import queryFirstTabbable from 'ally.js/dist/esm/query/first-tabbable.js';
import queryTabbable from 'ally.js/dist/esm/query/tabbable.js';
console.log({
whenKey,
whenVisableArea,
maintainHidden,
maintainDisabled,
maintainTabFocus,
queryFirstTabbable,
queryTabbable
});
and the following Rollup and webpack and configs:
import babel from 'rollup-plugin-babel';
import nodeResolve from 'rollup-plugin-node-resolve';
import commonjs from 'rollup-plugin-commonjs';
export default {
entry: 'main.js',
dest: 'dist/with-rollup.js',
plugins: [
nodeResolve(), // required to make Rollup understand how to load things from node_modules
commonjs(), // require to make Rollup load some of the CJS deps like platform, ect...
babel({
exclude: 'node_modules/**'
})
]
}
var path = require('path');
module.exports = {
entry: './main.js',
module: {
loaders: [
{
test: /\.js$/,
exclude: /(node_modules|bower_components)/,
loader: 'babel-loader'
}
]
},
output: {
filename: 'with-webpack.js',
path: path.resolve(__dirname, 'dist')
}
};
Webpack works great out of the box. Rollup builds successfully with the following warning:
⚠️ The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
https://github.com/rollup/rollup/wiki/Troubleshooting#this-is-undefined
node_modules/ally.js/node_modules/array.prototype.findindex/index.js (31:2)
29: Array.prototype.findIndex = findIndex;
30: }
31: }(this));
This means that Rollup essentially breaks The array.prototype.findindex
polyfill by passing undefined
instead of this
(window
). It looks like this is specific to the 1.0.0 array.prototype.findindex
Polyfill and might get fixed if we use the 2.0.0 version. In most cases though I think Array#findIndex will probally get defined by a user including core-js
or babel-polyfill
so we might be ok here.
I tried upgrading to 2.0.0 of array.prototype.findindex
but the tests failed with Error: Attempt to require unloaded module define-properties
. I attempted to add the new deps for v2 of the polyfill to the Intern loader paths but since array.prototype.findindex
is only shipped as CommonJS I think will need some more work so I have left it unchanged.
Nice work!
array.prototype.findindex
isn't used very often:
As far as I can see we should be able to replace that dependency by creating src/util/array-find-index.js
:
export default function(list, callback) {
const length = list.length;
for (let i = 0; i < length; i++) {
if (callback(list[i])) {
return i;
}
}
return -1;
}
I've been waiting for an excuse to remove array.prototype.findindex
and this seems to be it :)
Do you want to continue working on this?
Sure! Since you have been looking for an excuse I'll replace array.prototype.findindex
with a new utility function. I probably won't get to it today so look for a few more commits on Monday morning (Monday afternoon/evening your time).
array.prototype.findindex
is now gone as a dependency. I also removed all references to it in the doc, readme ect. My Rollup example now builds without errors or warnings! I also replaced the mention of the src
folder in the Getting Started docs with references to the new esm
folder.
Let me know if you want me to tweak anything else!
Looking good! The last tweaks I see:
refactor(util/array-find-index): replacing dependency array.prototype.findindex
and add a section to docs/api/util.md
and include a (small) test test/unit/util.array-find-index.js
chore(build): provide ES5 code as esm
and getting-started.md should not replace the ES6 source, but rather emphasize the ES5 code via ES6 modules. I think adding the primary use case (TypeScript app with webpack or rollup) would help clarify why this is necessaryDo you want take this to the finish line, or should I take over? (sorry for being a bit ridiculous here)
I would like to take this to the finish line if you are willing to wait until tomorrow for it 😄 . Just to make sure I understand:
util/array-find-index.js
into a single commit refactor(util/array-find-index): replacing dependency array.prototype.findindex
.getting-started.md
to emphasize ES5 code via ES6 modules and the use cases into a single commit chore(build): provide ES5 code as esm
.This PR should have 2 commits as a result:
refactor(util/array-find-index): replacing dependency array.prototype.findindex
, doc, tests, ect for removing array.prototype.findindex
.chore(build): provide ES5 code as esm
, new docs and build changes for shipping the esm
modules.I would like to take this to the finish line if you are willing to wait until tomorrow for it 😄.
<3
Just to make sure I understand:
correct :)
Ok, I think I have got everything squashed/rebased as you requested. Let me know if you want any more changes.
Thank you very much for your contribution! I've just released v1.4.1 containing your changes. :)
I've been using ally.js to handling accessibility in some Angular 2 components I have been making. I did run into a problem though when trying to build my final app with the Angular 2 Ahead-of-Time compiler. One of the compilers limitations is that all modules have to be in the ES 2015 module syntax and ally.js is distributed as UMD. Originally this wasn't a problem becuase ally.js was written in ES 2015 modules so I was able to do:
and everything worked, but since
ally.js
was coming fromnode_modules
it wasn't getting compiled down to ES5 to work in older browsers and Uglify also complained since Uglify JS can't minify most ES 2015 code.While I could have tried to reconfigure by build to also compile the
ally.js
source, it seemed better to add a build ofally.js
that had everything except the ES2015 modules compiled to ES5. Once this gets published to NPM you should be able to do the following in TypeScript, Rollup and webpack 2 with no loaders or plugins:To import the ES2015 modules that have been precompiled for you. This made including ally.js in my Angular 2 app as easy as:
I've also updated the build docs to reflect these change. Let me know if you want me to make any edits there.