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

Publish mosaic chains #75

Merged
merged 6 commits into from
Jun 11, 2019
Merged

Publish mosaic chains #75

merged 6 commits into from
Jun 11, 2019

Conversation

schemar
Copy link
Contributor

@schemar schemar commented May 24, 2019

Fixes #54

Packaging

A new script package.sh takes care of the packaging process.
It compiles the TypeScript into the ./lib directory as JavaScript.
It also copies the utility_chains directory. It has to copy the directory so that the relative imports from within the JavaScript files don't break.

Testing Packaging

The ./tests directory has a new script package.sh and a new directory package.
The ./tests/package.sh script packages the mosaic chains project and installs the created package into a test project in ./tests/package/.
It then accesses mosaic from node_modules/.bin/mosaic as well as from within a TypeScript file that imports the mosaic chains package.

This asserts that the code is accessible in executable form as well as as library.

package.json

I kept the version at 0.1.0-alpha.1. Happy to set anything else.

The bin, main, and files entries reference the compiled output inside ./lib.

Scripts

  • package is a new run script that packages mosaic chains
  • prepack ensures that packaging is executed before any run of npm pack or npm publish
  • test has the additional test:package command
  • test:package tests the packaging as described above

Web3 Version

Because we depend on @openst/mosaic.js, we share web3 as a peer dependency with that package. Therefore, we cannot upgrade to a newer version of web3 as that breaks @openst/mosaic.js.

version and description References

References from within the mosaic chains TypeScript files to the package.json file had to be removed. They resulted in a package.json file inside ./lib as part of the TypeScript compilation output. This ./lib/package.json was misleading the npm pack script and resulted in the wrong files being packaged.

Therefore, version and description can no longer be read from package.json. I removed it completely so that we won't have values that are out-of-sync in the future.

@schemar schemar marked this pull request as ready for review May 24, 2019 15:42
@schemar
Copy link
Contributor Author

schemar commented May 24, 2019

Ready for review 🦑

@schemar
Copy link
Contributor Author

schemar commented May 24, 2019

Closing and re-opening to trigger travis

Copy link
Contributor

@gulshanvasnani gulshanvasnani left a comment

Choose a reason for hiding this comment

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

Nice 👍
Please see one inline comment.
One question: why version and description is removed ?
I think we should also be able to publish mosaic-config for the existing chains.

package.sh Show resolved Hide resolved
tests/package.sh Outdated Show resolved Hide resolved
@gulshanvasnani gulshanvasnani removed their assignment May 27, 2019
@schemar
Copy link
Contributor Author

schemar commented May 27, 2019

Addressed all in-line comments.

One question: why version and description is removed ?

Please see the PR description.

Copy link
Contributor

@deepesh-kn deepesh-kn left a comment

Choose a reason for hiding this comment

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

Nice 💯
Overall LGTM.
Just a typo change.

tests/package.sh Outdated Show resolved Hide resolved
@deepesh-kn
Copy link
Contributor

@schemar, you should also consider publishing the mosaic configs. All config will be in a mosaic_config directory. The files name will like ropsten.json, kovan.json, 5020.json.. etc.

For now, you can consider them as empty JSON files. The actuals files will be added as they get available

@schemar
Copy link
Contributor Author

schemar commented May 28, 2019

Done, ready for review again 🦑

@deepesh-kn
Copy link
Contributor

Done, ready for review again 🦑

Tests are failing.

@schemar
Copy link
Contributor Author

schemar commented May 29, 2019

Will have a look. It seems another PR that was merged included wrong code so that compilation fails now. I did not work on the failing part.

@schemar
Copy link
Contributor Author

schemar commented May 29, 2019

Ready for review again after rebasing 🦑

@0xsarvesh 0xsarvesh self-assigned this May 29, 2019
Copy link
Contributor

@0xsarvesh 0xsarvesh left a comment

Choose a reason for hiding this comment

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

Nice 🚀

Two questions/feedback:

  1. Why do we expose these classes? Who is going to use them? they are more of internal implementation details of mosaic-chains in my opinion.
    Directory, Logger, Node, NodeDescription, NodeFactory, Shell

  2. How to access files published with mosaic-config directory? I try to publish this package locally using npm link but I couldn't access mosaic-config folder files. Am I missing something here? Or we need to expose mosaic-config files from index.ts?

@0xsarvesh 0xsarvesh removed their assignment May 29, 2019
@schemar
Copy link
Contributor Author

schemar commented May 29, 2019

Nice 🚀

Two questions/feedback:

  1. Why do we expose these classes? Who is going to use them? they are more of internal implementation details of mosaic-chains in my opinion.
    Directory, Logger, Node, NodeDescription, NodeFactory, Shell

I exposed all classes that were used in the executables, so that a consumer can write their own logic to run mosaic chains. For example, NodeDescription is crucial to create a node.

  1. How to access files published with mosaic-config directory? I try to publish this package locally using npm link but I couldn't access mosaic-config folder files. Am I missing something here? Or we need to expose mosaic-config files from index.ts?

Yes, you have to expose them in order for them to be available.

@0xsarvesh
Copy link
Contributor

Nice 🚀
Two questions/feedback:

  1. Why do we expose these classes? Who is going to use them? they are more of internal implementation details of mosaic-chains in my opinion.
    Directory, Logger, Node, NodeDescription, NodeFactory, Shell

I exposed all classes that were used in the executables, so that a consumer can write their own logic to run mosaic chains. For example, NodeDescription is crucial to create a node.

I can see mosaic binary is available to create a node, attach, list. I still don't understand how these classes will be used? What kind of logic they will write? Why can't they use executable?

  1. How to access files published with mosaic-config directory? I try to publish this package locally using npm link but I couldn't access mosaic-config folder files. Am I missing something here? Or we need to expose mosaic-config files from index.ts?

Yes, you have to expose them in order for them to be available.

Here, You == Consumer?
I think these files be should available with publishing. This approach is also discussed here #44 (comment)

@schemar
Copy link
Contributor Author

schemar commented May 29, 2019

Nice 🚀
Two questions/feedback:

  1. Why do we expose these classes? Who is going to use them? they are more of internal implementation details of mosaic-chains in my opinion.
    Directory, Logger, Node, NodeDescription, NodeFactory, Shell

I exposed all classes that were used in the executables, so that a consumer can write their own logic to run mosaic chains. For example, NodeDescription is crucial to create a node.

I can see mosaic binary is available to create a node, attach, list. I still don't understand how these classes will be used? What kind of logic they will write? Why can't they use executable?

As the ticket says, the idea is to publish mosaic chains as:

  1. executable
  2. library

Anyone can use the library to run mosaic chains from within their JavaScript code. E.g. integration tests.

  1. How to access files published with mosaic-config directory? I try to publish this package locally using npm link but I couldn't access mosaic-config folder files. Am I missing something here? Or we need to expose mosaic-config files from index.ts?

Yes, you have to expose them in order for them to be available.

Here, You == Consumer?
I think these files be should available with publishing. This approach is also discussed here #44 (comment)

you == package maintainer (us). How would the consumer have access? Whenever a new mosaic config is added, it must be exported from the index file.

@0xsarvesh
Copy link
Contributor

Nice 🚀
Two questions/feedback:

  1. Why do we expose these classes? Who is going to use them? they are more of internal implementation details of mosaic-chains in my opinion.
    Directory, Logger, Node, NodeDescription, NodeFactory, Shell

I exposed all classes that were used in the executables, so that a consumer can write their own logic to run mosaic chains. For example, NodeDescription is crucial to create a node.

I can see mosaic binary is available to create a node, attach, list. I still don't understand how these classes will be used? What kind of logic they will write? Why can't they use executable?

As the ticket says, the idea is to publish mosaic chains as:

  1. executable
  2. library

OK, Fine with me. I am not sure if logic written in mosaic command file should be considered for exposing to the consumer.

Anyone can use the library to run mosaic chains from within their JavaScript code. E.g. integration tests.

  1. How to access files published with mosaic-config directory? I try to publish this package locally using npm link but I couldn't access mosaic-config folder files. Am I missing something here? Or we need to expose mosaic-config files from index.ts?

Yes, you have to expose them in order for them to be available.

Here, You == Consumer?
I think these files be should available with publishing. This approach is also discussed here #44 (comment)

you == package maintainer (us). How would the consumer have access? Whenever a new mosaic config is added, it must be exported from the index file.

Another approach can be a small script which read all the files in the mosaic-config directory and expose them in the index.

@schemar
Copy link
Contributor Author

schemar commented May 29, 2019

Nice 🚀
Two questions/feedback:

  1. Why do we expose these classes? Who is going to use them? they are more of internal implementation details of mosaic-chains in my opinion.
    Directory, Logger, Node, NodeDescription, NodeFactory, Shell

I exposed all classes that were used in the executables, so that a consumer can write their own logic to run mosaic chains. For example, NodeDescription is crucial to create a node.

I can see mosaic binary is available to create a node, attach, list. I still don't understand how these classes will be used? What kind of logic they will write? Why can't they use executable?

As the ticket says, the idea is to publish mosaic chains as:

  1. executable
  2. library

OK, Fine with me. I am not sure if logic written in mosaic command file should be considered for exposing to the consumer.

It's like any other package that is an executable and a library. For example mocha:

I am obviously not able to explain this in a helpful way. Can someone else please try and explain it in their words? /cc @OpenST/maintainers

Anyone can use the library to run mosaic chains from within their JavaScript code. E.g. integration tests.

  1. How to access files published with mosaic-config directory? I try to publish this package locally using npm link but I couldn't access mosaic-config folder files. Am I missing something here? Or we need to expose mosaic-config files from index.ts?

Yes, you have to expose them in order for them to be available.

Here, You == Consumer?
I think these files be should available with publishing. This approach is also discussed here #44 (comment)

you == package maintainer (us). How would the consumer have access? Whenever a new mosaic config is added, it must be exported from the index file.

Another approach can be a small script which read all the files in the mosaic-config directory and expose them in the index.

Yes, that is another approach.

@deepesh-kn
Copy link
Contributor

Moving this to the parking lot for now.

@schemar
Copy link
Contributor Author

schemar commented Jun 4, 2019

Why can’t we merge this? 🤔

@deepesh-kn
Copy link
Contributor

Created a followup ticket for this
#90

cc: @schemar, @sarvesh-ost

@schemar
Copy link
Contributor Author

schemar commented Jun 10, 2019

Very good, I like #90 as a follow-up 👍

@0xsarvesh 0xsarvesh merged commit 6dda134 into OpenST:master Jun 11, 2019
@schemar schemar deleted the publish branch June 13, 2019 13:02
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.

Mosaic chains should be publishable
4 participants