Skip to content

Add initial code and dependencies#1

Merged
marcaaron merged 18 commits into
masterfrom
tgolen-initial-code
Nov 11, 2020
Merged

Add initial code and dependencies#1
marcaaron merged 18 commits into
masterfrom
tgolen-initial-code

Conversation

@tgolen

@tgolen tgolen commented Nov 4, 2020

Copy link
Copy Markdown
Collaborator

Held on https://github.com/Expensify/Expensify/issues/145764 while the repo is renamed.

This adds the initial Ion library.

Tests

This will be tested as part of Expensify/App#784

@tgolen tgolen self-assigned this Nov 9, 2020
@tgolen tgolen marked this pull request as ready for review November 10, 2020 22:00

@marcaaron marcaaron left a comment

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.

Couple thoughts...

  1. The source looks like it needs to be updated
  2. There was a conversation about changing the name of Ion to Onyx so maybe it would be a good time to make that change
  3. Can we build this with webpack and publish a distribution so it is easier to utilize by anyone? Should be pretty easy to do so I think https://webpack.js.org/guides/author-libraries/
  4. I'd expect that we'd also have withIon existing in this library and not only Ion so we can do things like
import Ion, {withIon} from 'react-ion';

Comment thread README.md

- Ion stores and retrieves data from persistent storage
- Data is stored as key/value pairs, where the value can be anything from a single piece of data to a complex object
- Collections of data are usually not stored as a single key (eg. an array with multiple objects), but as individual keys+ID (eg. `report_1234`, `report_4567`, etc.). Store collections as individual keys when a component will bind directly to one of those keys. For example: reports are stored as individual keys because `SidebarLink.js` binds to the individual report keys for each link. However, report actions are stored as an array of objects because nothing binds directly to a single report action.

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.

Maybe not needed right now, but we might want to be more neutral with our examples if this is getting open sourced.

Comment thread .editorconfig Outdated
# EditorConfig is awesome: http://EditorConfig.org

# Howto with your editor:
# Sublime: https://github.com/sindresorhus/editorconfig-sublime

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.

@tgolen

tgolen commented Nov 10, 2020

Copy link
Copy Markdown
Collaborator Author

Adding withIon makes sense, though I don't know that including a public distribution is necessary right now. I'd prefer to do both of those in separate PRs and just keep this initial PR simple.

I'll look into renaming Onyx, and I'll probably remove the sublime stuff completely just because I don't want to maintain a list of every editor and how to use .editorconfig (it's easy to google).

@tgolen

tgolen commented Nov 10, 2020

Copy link
Copy Markdown
Collaborator Author

Sounds like I am going to go ahead and rename it, yep. I'll have someone change the name of the repo. And since I'm renaming it, I'm also going to add and rename withOnyx and do it all in one swoop.

@tgolen tgolen changed the title Add initial code and dependencies [HOLD #145764] Add initial code and dependencies Nov 10, 2020
@marcaaron

Copy link
Copy Markdown
Contributor

Merge commit for JS-Libs here: Expensify/expensify-common@92b874e

@marcaaron marcaaron self-requested a review November 11, 2020 19:07

@marcaaron marcaaron left a comment

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.

LGTM! Thanks for doing this!!!

Just needs that JS-Libs version bump and we are good to go here.

One thing to follow up on (I'd be up for looking into this)...

eslint caught a couple errors so maybe we can add a GH action to run lint

Screenshot 1547

Comment thread package.json
"author": "Expensify, Inc.",
"homepage": "https://expensify.com",
"description": "State management for React Native",
"private": true,

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.

Probably will want to remove this when we publish. I think this flag just prevents publishing.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I figured we would leave this for now.

@marcaaron marcaaron changed the title [HOLD #145764] Add initial code and dependencies Add initial code and dependencies Nov 11, 2020
@marcaaron marcaaron merged commit 0cc1d0e into master Nov 11, 2020
@marcaaron marcaaron deleted the tgolen-initial-code branch November 11, 2020 19:42
mountiny pushed a commit that referenced this pull request Sep 15, 2023
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.

2 participants