parse-blockchain icon indicating copy to clipboard operation
parse-blockchain copied to clipboard

Refinement before alpha release

Open mtrezza opened this issue 4 years ago • 5 comments

Suggestions

  • [ ] Add @parse/blockchain-example as example for less manual setup --> https://github.com/parse-community/parse-server-blockchain/pull/6
  • [x] Change email address in the packages from Parse Community <admin@...>to Parse Platform <hello@...>, we will switch our public facing email to hello@.. over time, in some places its already changed.
  • [x] Remove "experimental" from package description; if we release it as 1.0.0-alpha.1 it implies that its experimental and we don't have to remove it again later.
  • [x] Release first versions as 1.0.0-alpha.1 to use an alpha tag semantically consistent with our other repos (may this this is a good repo to try out the monorepo / semantic release combination?)
  • [x] ~~Change server appID and masterkey to hello, world to work with parse dashboard OOTB~~ should change in dashboard repo
  • [ ] Change blockchain status to camelCase (e.g. sent instead of Sent)
  • [ ] Explain which contract address to use, truffle prints out 2 addresses
  • [ ] Rename repository to parse-blockchain? not sure about that, but it may be a better "umbrella name" for all the (future) blockchain related packages in this monorepo, some of which may not be directly parse-server related?
  • [x] Change package names from @parse/ethereum to something like @parse/blockchain-ethereum so if we have a list later with other networks they are grouped

If https://github.com/parse-community/parse-server-blockchain/pull/6 is not merged, these changes should also be done in the README:

  • [ ] Add step npm i @truffle/hdwallet-provider to readme

mtrezza avatar Oct 27 '21 19:10 mtrezza

@mtrezza when you say dashboard OOTB, are you referring to https://github.com/parse-community/parse-dashboard/blob/7c56b757dfb6cd1dd48c75968137c16a31a9565e/Parse-Dashboard/parse-dashboard-config.json? Actually even parse dashboard readme suggests the usage of port 1337, app id as myAppId, and master key as myMasterKey. Also, I don't believe that the right way to run parse dashboard is forking the repo and then running npm start, but, actually, npm install -g parse-dashboard, then parse-dashboard --dev --appId APPLICATION_ID --masterKey MASTER_KEY --serverURL "http://localhost:1337/parse" --appName MyAppName. We can add this command to the blockchain readme. What do you think? Maybe we should also change https://github.com/parse-community/parse-dashboard/blob/7c56b757dfb6cd1dd48c75968137c16a31a9565e/Parse-Dashboard/parse-dashboard-config.json to at least use the port 1337.

davimacedo avatar Nov 12 '21 05:11 davimacedo

About the status being camel case, I'm not sure I agree. It is currently an enum: https://github.com/parse-community/parse-blockchain/blob/master/packages/parse-blockchain/src/types.ts. Enum options use to be first letter capital case: https://www.typescriptlang.org/docs/handbook/enums.html. We can store it differently on db (maybe camel case, or lower case, or even caps lock). But I see no point in storing it differently in the db.

davimacedo avatar Nov 12 '21 05:11 davimacedo

I think all other items were addressed. Please let me know.

davimacedo avatar Nov 12 '21 05:11 davimacedo

Enum options use to be first letter capital case

I'm ok with TS defining the programming syntax but it shouldn't define the data syntax. For example, Swift switched from Capitalized to camelCase enum cases some time ago, but these are merely code syntax aesthetics that can change over time and be different depending on the coding language. It's dangerous to couple code syntax with data syntax because that relationship is not always obvious and changing code syntax should not influence data syntax. It could mean that a simple lint rule could cause data being written incorrectly to the DB. I would store it camelCase in the DB in any case (because data is usually stored like that) and split this off from any code syntax convention. We could just add a dbValue or rawValue method to the enum that returns the value as camelCased to write to the DB.

Actually even parse dashboard readme suggests the usage of port 1337, app id as myAppId, and master key as myMasterKey.

Ok, let's change that in the dashboard config. Regarding OOTB, I mean to just add dashboard as a dev dependency in the example package and add a npm script that starts dashboard with the correct params. That would make the curl commands in the blockchain Readme obsolete. So it's even easier to try out the blockchain example. But that's not really needed for a first release, maybe we can add that later on.

Rename repository to parse-blockchain? not sure about that, but it may be a better "umbrella name" for all the (future) blockchain related packages in this monorepo, some of which may not be directly parse-server related?

Sorry to roll this up again, I think my wording was confusing, now that I read it again. We have a verbose package tree now:

@parse/parse-server-blockchain
@parse/parse-server-blockchain/parse-blockchain
@parse/parse-server-blockchain/parse-blockchain-ethereum

I think I meant to rename @parse/parse-server-blockchain to @parse/parse-blockchain, because we may add packages that are not parse-server related to this parent package. I think we can simplify this even more with:

@parse/blockchain
@parse/blockchain/blockchain-server   // because it contains the server-related dependencies
@parse/blockchain/blockchain-ethereum

So under npm these packages would be listed as the following and the parent repo @parse/blockchain would not be on npm, correct?

@parse/blockchain-server
@parse/blockchain-ethereum

I know, a lot of renaming. What do you think?

mtrezza avatar Nov 12 '21 13:11 mtrezza

Hi @mtrezza . I did the renaming of the packages and also the camel case. Please let me know if we need something else in order to close this issue.

davimacedo avatar Dec 03 '21 01:12 davimacedo