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

Package up core-logger types #1833

Merged
merged 5 commits into from
Dec 19, 2018
Merged

Package up core-logger types #1833

merged 5 commits into from
Dec 19, 2018

Conversation

paroxysm
Copy link
Contributor

Proposed changes

Addresses #1807
Exports core-logger types to introduce type-safety to modules that depend on it.
According to https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html
We have two options, we can package up the types in the module itself leveraging https://basarat.gitbooks.io/typescript/docs/tips/barrel.html to export all types that other dependencies might reference OR publish a separate @types/<ark_module> to npm for each module. The former just works and resolves locally(w/o having to go through NPM to resolve things) so we can adopt that for the rest of the modules.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build (changes that affect the build system)
  • Docs (documentation only changes)
  • Test (adding missing tests or fixing existing tests)
  • Other... Please describe:

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@paroxysm paroxysm changed the base branch from master to develop December 17, 2018 18:25
@ghost ghost assigned faustbrian Dec 18, 2018
@ghost ghost added the review label Dec 18, 2018
@faustbrian
Copy link
Contributor

Tests are failing because of missing exports.

@paroxysm
Copy link
Contributor Author

Tests are now passing, the issue was that I hadn't included ./index.ts to be compiled in tsconfig.json

@faustbrian
Copy link
Contributor

Have you tried starting a relay after those changes?

@@ -6,7 +6,8 @@
"Brian Faust <brian@ark.io>"
],
"license": "MIT",
"main": "dist/index.js",
"main": "index",
Copy link
Contributor

Choose a reason for hiding this comment

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

This will result in an error that the module cannot be found if you try to start a relay as the compiled files are not what is being served for an import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is because now the resulting compiled files are in /dist/src as opposed to /dist previously. This is because we're now compiling ./index.ts, ts is preserving those folder structures when outputting to /dist
This causes that line pkg : require('../package.json') to fail because the file is now located in /dist/src/index.js as opposed to /dist/index.js.

It's fixable by going up 1 more level, so pkg : require('../../package.json') but it just reads weirdly...

Copy link
Contributor Author

@paroxysm paroxysm Dec 18, 2018

Choose a reason for hiding this comment

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

We can also impose a new pattern entirely, where in each module, the /src/index.ts is a barrel file for exporting types. Then we rename the file that exports the plugin descriptor object to plugin.ts or plugin.decl.ts to stand out.
That way, leave the tsconfig.js unchanged. All compiled sources explode into /dist instead of dist/src and it still works because the plugin.ts is still exporting the plugin property

Copy link
Contributor Author

@paroxysm paroxysm Dec 18, 2018

Choose a reason for hiding this comment

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

I've applied the changes I suggested @faustbrian, the relay now runs and all tests pass.
Lemme know if you're cool w/ this new pattern. The alternative is to combine both the barrel pattern and the existing code for exporting the plugin object into one index.ts but I wanted to keep the two separate

@faustbrian faustbrian merged commit ef2d321 into ArkEcosystem:develop Dec 19, 2018
@ghost ghost removed the review label Dec 19, 2018
@faustbrian
Copy link
Contributor

Looks ok for now, ultimately I would aim for proper declaration files though.

@paroxysm
Copy link
Contributor Author

paroxysm commented Dec 19, 2018

@faustbrian Based on the typescript compiler options that we're inheriting from @sindresorhus/tsconfig. Declaration files are being automatically generated. So if we're adopting a barrel file pattern, the corresponding .d.ts is generated for us.
This frees us from manually maintaining it. If we need to make tweaks, we can tweak the barrel file to exclude/include types

@faustbrian
Copy link
Contributor

Feel free to send PRs for other packages, the foreseeable future that solution is good enough.

@paroxysm
Copy link
Contributor Author

Okay cool, I wasn't sure whether it was an acceptable pattern to start applying, thanks for the go-ahead. I'll proceed with the others!

@paroxysm paroxysm deleted the feat/export-core-logger-types branch December 20, 2018 06:00
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