New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add additional build target of compiled es2015 mode w/ native es2015 module syntax #154

Merged
merged 2 commits into from Jan 31, 2017

Conversation

Projects
None yet
3 participants
@patrickarlt
Contributor

patrickarlt commented Jan 28, 2017

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:

import maintainTabFocus from 'ally.js/src/maintain/tab-focus.js';

and everything worked, but since ally.js was coming from node_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 of ally.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:

import maintainTabFocus from 'ally.js/esm/maintain/tab-focus.js';

To import the ES2015 modules that have been precompiled for you. This made including ally.js in my Angular 2 app as easy as:

// in a .d.ts file somewhere
declare module 'ally.js/dist/esm/maintain/tab-focus.js';
// in my app code
import maintainTabFocus from 'ally.js/esm/maintain/tab-focus.js';

I've also updated the build docs to reflect these change. Let me know if you want me to make any edits there.

@coveralls

This comment has been minimized.

coveralls commented Jan 28, 2017

Coverage Status

Coverage remained the same at 98.894% when pulling b952171 on patrickarlt:master into 28642d8 on medialize:master.

@rodneyrehm

This comment has been minimized.

Member

rodneyrehm commented Jan 28, 2017

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?

@patrickarlt

This comment has been minimized.

Contributor

patrickarlt commented Jan 28, 2017

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.

@patrickarlt

This comment has been minimized.

Contributor

patrickarlt commented Jan 29, 2017

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.

@rodneyrehm

This comment has been minimized.

Member

rodneyrehm commented Jan 29, 2017

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?

@patrickarlt

This comment has been minimized.

Contributor

patrickarlt commented Jan 29, 2017

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).

@coveralls

This comment has been minimized.

coveralls commented Jan 30, 2017

Coverage Status

Coverage increased (+0.007%) to 98.901% when pulling e2c6e42 on patrickarlt:master into 28642d8 on medialize:master.

@patrickarlt

This comment has been minimized.

Contributor

patrickarlt commented Jan 30, 2017

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!

@coveralls

This comment has been minimized.

coveralls commented Jan 30, 2017

Coverage Status

Coverage increased (+0.007%) to 98.901% when pulling 8418f39 on patrickarlt:master into 28642d8 on medialize:master.

@rodneyrehm

This comment has been minimized.

Member

rodneyrehm commented Jan 30, 2017

Looking good! The last tweaks I see:

  • e2c6e42 should be 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
  • 7d09514, b952171, 8418f39 should be 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 necessary

Do you want take this to the finish line, or should I take over? (sorry for being a bit ridiculous here)

@patrickarlt

This comment has been minimized.

Contributor

patrickarlt commented Jan 30, 2017

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:

  • Squash e2c6e42 and a new commit with doc and tests for the new util/array-find-index.js into a single commit refactor(util/array-find-index): replacing dependency array.prototype.findindex.
  • 7d09514, b952171, 8418f39 and a new commit which modifies 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.
@rodneyrehm

This comment has been minimized.

Member

rodneyrehm commented Jan 30, 2017

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 :)

@patrickarlt patrickarlt force-pushed the patrickarlt:master branch from 8418f39 to 343914f Jan 31, 2017

@coveralls

This comment has been minimized.

coveralls commented Jan 31, 2017

Coverage Status

Coverage increased (+0.007%) to 98.901% when pulling 343914f on patrickarlt:master into 28642d8 on medialize:master.

@patrickarlt

This comment has been minimized.

Contributor

patrickarlt commented Jan 31, 2017

Ok, I think I have got everything squashed/rebased as you requested. Let me know if you want any more changes.

@rodneyrehm rodneyrehm merged commit 93e3332 into medialize:master Jan 31, 2017

3 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.007%) to 98.901%
Details
@rodneyrehm

This comment has been minimized.

Member

rodneyrehm commented Jan 31, 2017

Thank you very much for your contribution! I've just released v1.4.1 containing your changes. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment