amazon-ion / ion-js Goto Github PK

As testing harness spec is fleshed out IonJS needs to be kept abreast of the necessary endpoints/logic to support interoperability in the future.

Get rid of var, replace everywhere with let

A constant named "ERROR" is used as a sentinel value to indicate that the lexer (ParserTextRaw) is currently not holding on to a value, but this is confusing as this is a normal state after each call to next() and thus doesn't actually indicate any error. Should most likely be renamed to NONE

From the main page at https://github.com/amzn/ion-js

var ionJs = require("ion-js")

var ionData = "{ hello: \"Ion\" }"; 
var ionReader = ionJs.makeReader(ionData); 
ionReader.next(); 
ionReader.stepIn(); 
ionReader.next(); 
var hello = ionReader.fieldName(); 
var ion = ionReader.stringValue(); 
ionReader.stepOut(); 
console.log(ion.concat(", ").concat(hello));

This is the output:

"ello: \"Ion, hello"

Pretty sure the output is supposed to be "Ion, hello"

Tested using:

1. https://amzn.github.io/ion-js/browser/scripts/ion-bundle.min.js (recommended on the README)
2. https://npm.runkit.com/ion-js ("Try it yourself" on the README)

Looks like parsing of strings inside of structs is broken:

var ionJs = require("ion-js");
var r = ionJs.makeReader('{key:"value"}');
r.next(); r.stepIn(); r.next();
console.log("fieldName: " + r.fieldName());
console.log("stringValue: " + r.stringValue());

// output:
"fieldName: key"
"stringValue: ey:\"value"

The API supported bu Ion-js mimics (it is inspired by) the other programming language libs for Ion.
it is a little verbose to use.

Devs tends to create their Ion data as a String in the native language and then pass that whole string to the lib to build the Ion Value. This makes it hard to catch errors in the input string.

Maybe provide a more fluent mechanism to create Ion values could be of more help.

Using Reader.next() on Ion Data currently returns a value of type IonType.

The return value seems internal to me. What should (or could) a client do with a value of IonType?

The code is close to but not exactly ready for a Node.js package. Finish this up and create a Node.js pacakge for IonJS

Should take a quick peek at the terminology and API of, say, the Java implementation to ensure our readers and writers work similarly (in particular I'm thinking about entering and leaving containers).

Currently IonTextReader exposes annotations via .annotations() (returns an Array), whereas IonBinaryReader exposes annotations via hasAnnotations() and getAnnotation(i) (returns the annotation at a specific index in the array).

Can these be unified into a common API on the Reader interface?

This might be relevant to #29.

We don't want the initial release to be 1.0.0, it should be more beta-ish

The boolean argument to _read_string_helper should be promoted to an enum for clarity when calling the method

Testing framework has no logic around asserting equality, need to bring testing framework inline with the more spec compliant ionjava/ionc frameworks.

Current setup spits out ES6 .js files. Figure out how to get ES5 support going as well.

Booleans should be enums for clarity

Once we create an IonWriter and start writing values into our Ion data it would be nice to be able to return a formatted string of the whole value

This type constant is unclear - what does the 2 refer to? Needs a better name

Even though there is data loss when translating from Ion to JSON, the ability to do so seems to be a good feature to have here. So adding operations that will allow

• JSON->Ion
• Ion ->JSON

as part of the API would be nice.

Testing currently fails silently indicating a disconnect between the interface/unittest contract in our testing suite. To create a comprehensive buglist we need to restructure how testing/exception handling works.

I think I made a goof while introducing type safety and made the argument to makeReader() a Span. Ideally users should just have some data (string, number[], Buffer, etc.) and be able to pass it directly into makeReader() - we should do all the heavy lifting on our part to wrangle it into the correct Span type

The current tutorial for using Ion inside the browser as a script (not as an npm package) is primitive.

The tutorial should walk through the basics of Ion and the manipulation of Ion values through Ion-js with inline runnable examples.

I'm unsure if this is a regression or if it was never intended behavior in the first place.

Previously, passing text data as a byte array to makeReader would still return a functioning text reader. Now it throws an error:

var ionJs = require("ion-js")
var input = "{foo:bar}";
var arr = new Uint8Array(input.length)
// A more realistic example would be loading a text file from disk as a Buffer (node.js) or Blob (HTML5)
for (var i = 0; i < input.length; i++) arr[i] = input.charCodeAt(i)

var reader = ionJs.makeReader(arr);
console.log(reader.next().name);

TypeError: this._src.charCodeAt is not a function

2.0.0: https://runkit.com/embed/vz9xv8h87vqp
2.1.2: https://runkit.com/embed/ee8ubusq2qrx (contains more detailed stack trace)

Added roundtrip tests to ensure that the library parses to the correct values.

• UML diagram documenting interaction of files/objects/methods within the codebase.

• Refactoring inheritance using JS equivalents of "Interfaces/classes" to allow typescript to ensure the contracts detailed in the UML. This allows for complete code coverage for contracts at runtime by the typescript compiler.

• Explicit comments describing each file/piece of functionality as it is refactored.

Currently we use a block of logic to select from a couple different version of JavaScript in our tests:

define([
'intern',
'intern!object',
'intern/chai!assert',
'dist/amd/es5/IonTests',
'dist/amd/es6/IonTests',
],
function(intern, registerSuite, assert, ionEs5, ionEs6) {
var ionVersions = {
es5: ionEs5,
es6: ionEs6,
};
var ion = ionVersions[intern.args.ionVersion];

A more clever way would be to have a module (tests/unit/Version.js) that encapsulates this switching logic:

define(
['intern', 'dist/amd/es5/IonTests', 'dist/amd/es6/IonTests'],
function(intern, ionEs5, ionEs6) {
var ionVersions = {
es5: ionEs5,
es6: ionEs6,
};
return ionVersions[intern.args.ionVersion];
}
);

Also, the "ionVersion" parameter name is misleading and should be changed.

With that module, the above switching logic is extracted away and our test includes are now simply:

define([
'intern!object',
'intern/chai!assert',
'tests/unit/Version',
],
function(registerSuite, assert, ion) {

Configure travis and integration builds for ion-js. Ensure that documentation and reporting are configured for public browsing through Github Pages.

Document release process so that anyone with write permission can perform a release.

The constant CH_DQ should be renamed to be CH_DOUBLE_QUOTE for clarity

Reader seems to parse symbols given as JS strings with escaped single quotes as String instead of Symbol.

> require("ion-js").makeReader("\'sym\'").next()
IonType {
bid: 8,
name: 'string',
scalar: true,
lob: false,
num: false,
container: false }

The above should have yielded a Symbol not a String

Contrast the expected behavior with double quoted strings and bareword/identifier symbols:

> require("ion-js").makeReader("\"str\"").next()
IonType {
bid: 8,
name: 'string',
scalar: true,
lob: false,
num: false,

> require("ion-js").makeReader("identifier").next()
IonType {
bid: 7,
name: 'symbol',
scalar: true,
lob: false,
num: false,
container: false }

Code:

var reader = ionJs.makeReader("'quoted_annotation'::{}")
reader.next()
console.log(reader.annotations())
while (reader.annotations().shift()) ; // Workaround for separate bug of static instance

var reader = ionJs.makeReader("first::'second'::{}")
reader.next()
console.log(reader.annotations())

In 2.0.0 (behaves as expected): https://runkit.com/embed/4kzxn7to1c9f

["quoted_annotation"]
["first", "second"]

In 2.1.2 (latest): https://runkit.com/embed/lri33z6vznsp

["'quoted_annotation"]
["first", "irst::'second"]

Expand name for clarity

Annotations should not accumulate between new instances of IonReader.

r = require("ion-js").makeReader("ann1a::ann1b::10 ann2::11"); r.next(); r.annotations()
[ 'ann1a', 'ann1b' ]
> r = require("ion-js").makeReader("ann1a::ann1b::10 ann2::11"); r.next(); r.annotations()
[ 'ann1a', 'ann1b', 'ann1a', 'ann1b' ]
> r = require("ion-js").makeReader("ann1a::ann1b::10 ann2::11"); r.next(); r.annotations()
[ 'ann1a', 'ann1b', 'ann1a', 'ann1b', 'ann1a', 'ann1b' ]

Should verify reader's beahviour when for the same reader instance we navigate to multiple values that have annotations. The API should return the annotations of the current value being pointed to by the reader.

Document the process by which PR are to be created, reviewed and incorporated.

It may be worthwhile to leverage ASM.js for low-level serialization bits. This could be done as handcoded routines, or using some tooling. I'd imagine binary encoding/decoding could get some efficiency from this.

Currently the Typescript code is used to generate

• es6 code (this is what we currently run our tests against)
• commonjs modules that create the npm package
• minified es5 code for use in the browser

There are no automated tests on the resulting js code. The tools are pretty mature but still we should have automated tests such that

• npm package is tested (pack, install and run)
• minified js bundle is tested on browser(s)

"double" here refers to double quotes, not a numeric precision. Should be renamed to be more clear

Todo: add support for bad and equivs test suites.

Currently Ion.ts exports a bunch of classes just for test visibility. We may want to look into having two sets of exports, one for consumers and one for tests so that we're not leaking too many implementation details

Trying to parse 2017-05-01T01:01:00.000Z timestamp will fail with the following error.

Error: Invalid decimal 00.000Z
  at Function.parse  </Users/user/ion-js/src/IonDecimal.ts:188:12>
  at Function.parse  </Users/user/ion-js/src/IonTimestamp.ts:504:28>
  at Test.suite.(anonymous function) [as test]  <tests/unit/IonTimestampTest.js:26:23>

Currently the build generates .js files from .ts files. Add the step to minify (or uglify) the generated .js files as well. This might require moving to ES5.

Make sure that instead of just navigating over values we are actually materializing them (getInteger(), getTimestamp(), or whatever they're called)

It appears that the current process of ES6->ES5 does not deal with Object.assign

To repro

1. Run jjs
2. load("https://amzn.github.io/ion-js/browser/scripts/ion-bundle.js")

Thows
https://amzn.github.io/ion-js/browser/scripts/ion-bundle.js:5481 TypeError: Object.assign is not a function

Try the corresponding polyfill
https://www.npmjs.com/package/babel-plugin-transform-object-assign

We should be explicit about the limitations of our implementation. In particular, the use of Javascript Number type basically means we're limited to ~53-bits of integer. There are probably other limitations worth noting around things like Decimal as well.

I have the following Ion structure which was properly created in Java:

'[email protected]'::{
  start_date:2017-05-01T00:00:00.000Z,
  end_date:2017-06-01T00:00:00.000Z
}

When reading this data, ion-js fails with the following error message.

Error: Hour 0 must be between 1 and 23 inclusive
    at Timestamp.checkValid (/ion-js/dist/commonjs/es6/IonTimestamp.js:242:27)
    at new Timestamp (/ion-js/dist/commonjs/es6/IonTimestamp.js:220:14)
    at read_timestamp_value (/ion-js/dist/commonjs/es6/IonParserBinaryRaw.js:358:9)
    at ParserBinaryRaw.load_value (/ion-js/dist/commonjs/es6/IonParserBinaryRaw.js:573:21)
    at ParserBinaryRaw.timestampValue (/ion-js/dist/commonjs/es6/IonParserBinaryRaw.js:756:15)
    at BinaryReader.timestampValue (/ion-js/dist/commonjs/es6/IonBinaryReader.js:168:29)

3 refers to "triple quoted", name should be expanded so this is obvious

CH_SQ, CH_OC, etc.

These are unreadable

Fixy fixy

• The record has a timestamp field startDate = 1498415400000.
• This is GMT: Sunday, June 25, 2017 6:30:00 PM.
• When creating an ion struct in Java, a corresponding field was added that reflects the timezone like start_date:2017-06-26T00:00:00.000+05:30

1. The problem is that when reading this value with ion-js, the stringValue() produces an incorrect mixed date value 2017-06-25T18:30:0.0+05:30 which is neither the UTC nor the locatime.
2. Also notice how seconds are formatted. They should be 00.000 instead of 0.0.

The current API for readers involves making a span and then passing it into a method to make the actual reader ( makeReader(makeSpan("{foo:bar")) ). This unnecessarily exposes internal details (although, I can imagine custom implementations of the Span interface that go beyond the basic number[] and String types) and overcomplicates the API. makeReader's first parameter should be String | number[] | Span and delegate to makeSpan accordingly such that callers need only do makeReader("{foo:bar}")

type Operator = () => void;