Skip to content

Pioneer media app fixes and types updates#687

Merged
mnaamani merged 13 commits intoJoystream:nicaeafrom
mnaamani:pioneer-media-app-fixes
Jun 16, 2020
Merged

Pioneer media app fixes and types updates#687
mnaamani merged 13 commits intoJoystream:nicaeafrom
mnaamani:pioneer-media-app-fixes

Conversation

@mnaamani
Copy link
Member

@mnaamani mnaamani commented Jun 12, 2020

Part of #663 - excluding resolver cache improvements

  • Update DataObject and DataObjectStorageRelationship types
  • Change version to 0.11.0
  • Use relative path to @joystream/types package by dependents in workspace
  • Update dependents in repo to use path instead of version
  • Drop SchemaId type
  • Drop ContentVisibility type
  • Removed old types definitions from unused old module
  • Refactor types/index.ts - move council and election types into separate file, common types into common.ts
  • Updated pioneer after refactor of types

Tried to identify as many cases of circular references as possible.

@mnaamani
Copy link
Member Author

I'm currently getting error in web console when running pioneer:

TypeError: class heritage _members.ActorId is not an object or null

Will investiage

@mnaamani
Copy link
Member Author

Still work in progress, after all the refactoring in types will need to update pioneer.

@mnaamani mnaamani requested a review from Lezek123 June 12, 2020 22:38
@mnaamani mnaamani marked this pull request as ready for review June 12, 2020 22:38
Copy link
Contributor

@Lezek123 Lezek123 left a comment

Choose a reason for hiding this comment

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

The circular dependency problem is solved. I went through the changes and tested a few things:

  • Checked almost all CLI commands (including the new working-groups one I merged into this branch) - found no issues
  • Tested Pioneer's forum, proposals, council election process etc. (functionalities where file changes were included) - found no issues
  • Tested whether merging my new cli and Pioneer related changes doesn't cause any issues - it doesn't
  • Whether this fixes the issue I encountered in joystream-api-examples - it does (but an update in that repository is required)
  • Checked whether circular dependency detection tool doesn't detect any circular dependencies anymore - I used madge tool here and it didn't detect any circular dependencies. I then purposefully created a circular dependency and re-build the types and it was succesfully detected, so the tool seems to be working fine.
  • Ran network tests (had to run yarn install first though) - there were a few issues with transaction priority and timeouts, but didn't seem like anything related to the changes in this PR
  • Didn't fully test the media app yet as it would probably require some more setup (and I'm not sure if that's even possible already?)

I was thinking that it would be nice to have some additional CI checks included (not necessarily in this PR), ie.:

  • yarn workspace @joystream/types build (now that they are not part of Pioneer workspace anymore)
  • A script like yarn workspace @joystream/types build && madge --circular types to avoid circular dependencies in the future
  • yarn workspace joystream-cli build (just to make sure CLI code compiles) Edit: that's actually only possible after #692 gets merged

"bugs": "https://github.com/Joystream/substrate-runtime-joystream/issues",
"dependencies": {
"@joystream/types": "^0.10.0",
"@joystream/types": "./types",
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this cause a problem if we decide to publish this package? (Similarly to "@joystream/types": "*"?)

Copy link
Member Author

@mnaamani mnaamani Jun 15, 2020

Choose a reason for hiding this comment

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

Yes it would indeed. I think we need to investigate what is the best practice inside a workspace. I did make you change a similar import once with '*' for the same reason :)
We really need to look into lerna and if it perhaps helps to resolve the correct version at the point when we actually publish.

@Gamaranto is exploring using lerna now while working on atlas.

It is worth mentioning also that he used it to build packages in the correct order based on the dependency tree. Maybe it could also help to detect circular dependencies in general?

@mnaamani
Copy link
Member Author

I was thinking that it would be nice to have some additional CI checks included (not necessarily in this PR), ie.:

* `yarn workspace @joystream/types build` (now that they are not part of Pioneer workspace anymore)

* A script like `yarn workspace @joystream/types build && madge --circular types` to avoid circular dependencies in the future

* `yarn workspace joystream-cli build` (just to make sure CLI code compiles) _Edit: that's actually only possible after #692 gets merged_

Yes, I had an issue for nicea to add some new CI checks here #664 will add your suggestions to it.

@mnaamani mnaamani merged commit 4eb10c3 into Joystream:nicaea Jun 16, 2020
@mnaamani mnaamani deleted the pioneer-media-app-fixes branch June 16, 2020 09:34
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