Skip to content

Use parse/stringify from flatted lib to support circular structures (fixes #222)#240

Merged
nicolodavis merged 6 commits into
boardgameio:masterfrom
Stefan-Hanke:ser_deser_circular
Jul 25, 2018
Merged

Use parse/stringify from flatted lib to support circular structures (fixes #222)#240
nicolodavis merged 6 commits into
boardgameio:masterfrom
Stefan-Hanke:ser_deser_circular

Conversation

@Stefan-Hanke
Copy link
Copy Markdown
Contributor

This PR fixes the problem described in #222. It replaces the use of JSON.stringify/JSON.parse with functions from the flatted library.

Instead of just fixing that problem, this PR changes stringify/parse to flatted in all parts that consitute the library itself - i.e. the examples and the storybook are unchanged.

Checklist

  • Use a separate branch in your local repo (not master).
  • Test coverage is 100% (or you have a story for why it's ok).

Comment thread src/client/debug/debug.js Outdated
import { DebugMove } from './debug-move';
import { GameLog } from '../log/log';
import { restore } from '../../core/action-creators';
import { parse, stringify } from 'flatted/cjs';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be flatted/esm

From https://github.com/WebReflection/flatted#flatted

// ESM
import {parse, stringify} from 'flatted/esm';

// CJS
const {parse, stringify} = require('flatted/cjs');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I aggree. I changed to esm locally, and changed the rollup config accordingly. However, now tests do not run anymore because they fail to resolve the "export" keyword inside flatted/esm/index.js.

Somehow jest now needs to be told to transform ES6 modules also. Likely another change in .babelrc is necessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So how was I able to push this branch at all - it should have been caught by the prepush hook? Since I can't push the esm variant because now tests are failing :(

As far as I understand this, it should work since .babelrc contains transform-es2015-modules-commonjs inside test env directly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can push even if tests are failing using: git push --no-verify

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except I didn't.

Copy link
Copy Markdown
Member

@nicolodavis nicolodavis Jul 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes tests can pass locally but fail on Travis if it depends on something in node_modules that's set up correctly locally but incorrectly in package.json.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I thought your question was about how to push now that the tests are failing.

@coveralls
Copy link
Copy Markdown

coveralls commented Jul 24, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 8e48165 on Stefan-Hanke:ser_deser_circular into edd1df0 on google:master.

@Stefan-Hanke
Copy link
Copy Markdown
Contributor Author

Well and now one cannot run the dev server anymore because it complains that "export" keyword is unexpected inside flatted/esm.

@nicolodavis
Copy link
Copy Markdown
Member

Will take a look.

@Stefan-Hanke
Copy link
Copy Markdown
Contributor Author

As a sidenote, why is there a separate coverage check when the Travis build also checks coverage?

@nicolodavis
Copy link
Copy Markdown
Member

Fixed by just importing:
flatted instead of flatted/esm or flatted/cjs.

Their package.json includes a main and module section, which should allow imports to pick up the correct file.

About the coverage check, are you talking about why we use Coveralls in addition to Travis? Travis merely runs the build and reports the coverage to Coveralls, which has better tooling like the ability to view covered / uncovered lines. In theory it is possible to have Travis just spit out a coverage number and fail the build if it doesn't meet a threshold, but Coveralls is useful enough that I decided to keep them separate.

@nicolodavis nicolodavis merged commit 239f8dd into boardgameio:master Jul 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants