amazon-ion / ion-js

A JavaScript implementation of Amazon Ion.
http://amzn.github.io/ion-docs/
Apache License 2.0
352 stars 48 forks source link

Loading ion-js breaks standard Webpack build #116

Open cimi opened 6 years ago

cimi commented 6 years ago

I'm trying to include ion-js in a project that uses a standard webpack build (create-react-app):

import { makeReader } from 'ion-js';

console.log('Hello world!');

See the full repo here, this is the commit that introduces ion-js.

Trying to bundle the project fails with the following error:

➜  ion-js-webpack-demo git:(master) yarn build
yarn run v1.3.2
$ react-scripts build
Creating an optimized production build...
Failed to compile.

Failed to minify the code from this file:

    ./node_modules/ion-js/dist/commonjs/es6/IonUtilities.js:27

Read more here: http://bit.ly/2tRViJ9

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

This is because the loader does not support ES6 modules by default. While this can be probably worked around by ejecting the default configuration and customising the webpack build configuration, it would be much more convenient if ion-js produced an ES5 version that can be imported directly. The ES5 AMD bundle generated in the current build setup is meant to be included standalone in a browser and does not work with module loaders.

cimi commented 6 years ago

I've opened PR #117 which makes the TypeScript compile step output ES5 alongside ES6. It updates package.json with a browser field that instructs module loaders to use the ES5 output as the main entry point when working with this package as a dependency. It requires no other changes to the source or to the way the package is included and should be transparent to all other customers.

It would have been possible to use babel to convert the CommonJS ES6 output to ES5, however this introduces complexity through an extra translation step and (at least in development mode) requires the babel polyfill to be available in order to import the ion library. Without the polyfill, referencing regeneratorRuntime yields errors:

babel-regenerator-error

I've managed to go around that by adding an import "babel-polyfill"; at the top of the Ion.ts/Ion.js entry point but this is again, added complexity and should be avoided if possible.

I think the amd ES5 output could be emitted directly by the TypeScript compiler using a similar set of options, which would mean babel could be removed from the project. I did not make this change though because I am not sure if there are any other use-cases for which babel was imported to the project.

wesboyt commented 6 years ago

Hey cimi, thanks for getting involved with ion-js! I would recommend checking out commit 3f80e64d7d87ec37fa293f7d39386addd9144cda There were breaking changes introduced to the library after that commit that will corrupt your data. We use grunt to build our library normally but are looking at setting the tsc config outside of the gruntfile so that other builds will have access to our compiler options. Will update when we do our next release and the library is in a more stable state. -Wesley

cimi commented 6 years ago

Thanks for your quick reply!

I would recommend checking out commit 3f80e64

What do you mean? I intend to consume the package via npm/package.json so if that commit is not tagged and published I won't be able to use it. Also, for that specific commit, the CI log says it failed to build?

There were breaking changes introduced to the library after that commit that will corrupt your data.

From the commit history I see that most (all?) subsequent changes were related to IonText - I need to use only the binary API and I have tests for my intended use-cases so I guess I'm safe? 😅

We use grunt to build our library normally but are looking at setting the tsc config outside of the gruntfile so that other builds will have access to our compiler options.

How would this process look then? My application is not using TypeScript and I consume ion-js as a prebuilt module. If the entry point given to my module bundler for ion-js is not ES5 compatible I will be getting the same error regardless if the build options are in the Gruntfile or outside. (Please let me know if I'm missing something).

Cheers! Alex

wesboyt commented 6 years ago

Hey Cimi sorry about the delayed response, I didn't notice your comment. A few months ago we started expanding the testing suite and realized how incredibly broken the library actually is. We actually changed its state to alpha and will be rebuilding large sections from scratch to ensure a product up to our standards internally. We have not begun fixing the binary side of the library and the text side will probably not be released for a few more months as multiple sections have been found to be non spec compliant. As for the PR we will be entirely reverting a huge section of commits and will try to merge your additions into the release at that time but it will be at least a few more months.