Skip to content
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

Extract transforms for lerna #1420

Merged
merged 11 commits into from Feb 8, 2017

Conversation

rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Feb 7, 2017

Extract our transforms from media type payloads ➡️ React Elements.

@ivanov ivanov force-pushed the extract-transforms-for-lerna branch from f98a330 to 6f72b6d Compare February 7, 2017 23:17
@ivanov
Copy link
Member

ivanov commented Feb 7, 2017

rebased, for starters.

"@nteract/transform-model-debug": "1.0.0",
"@nteract/transform-plotly": "1.0.0",
"@nteract/transform-vega": "1.0.0",
"@nteract/transforms": "1.0.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

❤️ this is definitely moving along

@ivanov
Copy link
Member

ivanov commented Feb 8, 2017

Tests pass for me locally, let's see what our CI pals have to say about it.

@ivanov
Copy link
Member

ivanov commented Feb 8, 2017

Oh, I think it's because I ran lerna locally, which CI doesn't yet know about. Let me add that.

@ivanov
Copy link
Member

ivanov commented Feb 8, 2017

I think the flow errors we're seeing are similar to some of the issues reported on this thread: immutable-js/immutable-js#203 but I have not followed it all to figure out how to get around it.

@ivanov
Copy link
Member

ivanov commented Feb 8, 2017

I don't really know how flow works, but might it have something to do with the fact that we have 3 copies / version of immutable.js?

$ find . -name immutable.js.flow
./node_modules/immutable/dist/immutable.js.flow
./packages/commutable/node_modules/immutable/dist/immutable.js.flow
./packages/transforms/node_modules/immutable/dist/immutable.js.flow

so the lerna-ed packages reference their own, but the main app references its own, and flow doesn't understand that they're the same?

@rgbkrk
Copy link
Member Author

rgbkrk commented Feb 8, 2017

I'm looking at the flow types for Immutable and the getIn definition is very wrong:

getIn<T>(searchKeyPath: ESIterable<any>, notSetValue: T): T;
getIn<T>(searchKeyPath: ESIterable<any>): T;

A deeply nested value may not actually be T.

@peggyrayzis what's the best way for us to ignore bad parts of definitions that come as part of the library (not in our flow-typed directory)?

@rgbkrk
Copy link
Member Author

rgbkrk commented Feb 8, 2017

Then again, this was passing on master.

Have you run into anything like this @thejameskyle or @hzoo? We're trying to adapt to using lerna (amidst shipping an Electron app), running into trouble with the flow definitions for Immutable.

@ivanov
Copy link
Member

ivanov commented Feb 8, 2017

For what it's worth, I get the same flow error on master. :\

@ivanov
Copy link
Member

ivanov commented Feb 8, 2017

Ok, so here's how the errors can be eliminated: lerna bootstrap --hoist immutable... - This will not put immutable in the packages/*/node_modules, but only in the top level node_modules.

except I think there's a bug in lerna, because running that with the --hoist immutable flag it just hangs there...

symlinking the root node_modules/immutable to the packages/commutable/node_modules/immutable also fixes the flow stuff. As does removing the latter after a lerna bootstrap by hand.

@codecov-io
Copy link

codecov-io commented Feb 8, 2017

Codecov Report

Merging #1420 into master will increase coverage by 0.01%.

@@            Coverage Diff             @@
##           master    #1420      +/-   ##
==========================================
+ Coverage   90.42%   90.44%   +0.01%     
==========================================
  Files          66       67       +1     
  Lines        1724     1726       +2     
==========================================
+ Hits         1559     1561       +2     
  Misses        165      165

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00454a5...8de3e73. Read the comment docs.

@peggyrayzis
Copy link
Member

@peggyrayzis what's the best way for us to ignore bad parts of definitions that come as part of the library (not in our flow-typed directory)?

It looks like Paul already fixed the issue, but I would put a line in the [ignore] section of .flowconfig like .*/node_modules/immutable/dist/.*. This is an OCaml regex, so it can can pick up on any subdirectories, like the directories in packages/, that also contain this file pattern.

@rgbkrk
Copy link
Member Author

rgbkrk commented Feb 8, 2017

For what it's worth, I get the same flow error on master. :\

Ah, that must be because you and I have both done a lerna bootstrap, which means that it's resolving the Immutable in its node_modules.

@rgbkrk
Copy link
Member Author

rgbkrk commented Feb 8, 2017

It looks like Paul already fixed the issue, but I would put a line in the [ignore] section of .flowconfig like ./node_modules/immutable/dist/.. This is an OCaml regex, so it can can pick up on any subdirectories, like the directories in packages/, that also contain this file pattern.

Tried it, didn't work. 😭

@rgbkrk
Copy link
Member Author

rgbkrk commented Feb 8, 2017

I wonder if this is because we listed it as a dev dependency:

lerna/lerna#505

We could switch back to a normal dependency, the reason I didn't want to go that route was so that people could rely on it as a peer dependency.

@rgbkrk
Copy link
Member Author

rgbkrk commented Feb 8, 2017

I'm trying some things and will post a new commit when I think it's in a friendly state.

@rgbkrk rgbkrk changed the title [WIP] Extract transforms for lerna Extract transforms for lerna Feb 8, 2017
@rgbkrk
Copy link
Member Author

rgbkrk commented Feb 8, 2017

It passes! 🔨

package.json Outdated
@@ -31,7 +31,7 @@
"lint": "eslint src/ test/",
"lint:fix": "eslint . --fix",
"prebuild": "rimraf lib",
"lerna": "lerna bootstrap --hoist immutable",
"lerna": "lerna bootstrap && rimraf packages/commutable/node_modules/immutable",
Copy link
Member Author

Choose a reason for hiding this comment

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

This will be a stopgap for us until the next lerna release (which will probably be today).

Copy link
Member

Choose a reason for hiding this comment

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

yeap, I opened #1436 so we remember to do it in case lerna release gets held up.

@rgbkrk
Copy link
Member Author

rgbkrk commented Feb 8, 2017

Whoops, because of me there's a merge conflict with the geojson transform. I'll handle that now.

maybe CI will have better luck than what I get here.
@rgbkrk rgbkrk force-pushed the extract-transforms-for-lerna branch from ba9454e to 28ee93d Compare February 8, 2017 19:44
@rgbkrk
Copy link
Member Author

rgbkrk commented Feb 8, 2017

Rebased, brought in the background color -> GeoJSON tile support.

@rgbkrk
Copy link
Member Author

rgbkrk commented Feb 8, 2017

Yay, new lerna release fixes it for us! Thanks all.

Copy link
Member

@peggyrayzis peggyrayzis left a comment

Choose a reason for hiding this comment

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

Everything looks great to me! I'm excited to start working on exporting packages 🎉

@peggyrayzis peggyrayzis merged commit f3ed729 into nteract:master Feb 8, 2017
@rgbkrk rgbkrk deleted the extract-transforms-for-lerna branch February 8, 2017 21:09
@lock
Copy link

lock bot commented Apr 3, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked and limited conversation to collaborators Apr 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants