Skip to content

Package up core-logger types#1833

Merged
faustbrian merged 5 commits intoArkEcosystem:developfrom
paroxysm:feat/export-core-logger-types
Dec 19, 2018
Merged

Package up core-logger types#1833
faustbrian merged 5 commits intoArkEcosystem:developfrom
paroxysm:feat/export-core-logger-types

Conversation

@paroxysm
Copy link
Copy Markdown
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
Comment thread packages/core-logger-winston/src/index.ts Outdated
Comment thread packages/core-logger/package.json Outdated
@ghost ghost assigned faustbrian Dec 18, 2018
@ghost ghost added the review label Dec 18, 2018
@faustbrian
Copy link
Copy Markdown
Contributor

Tests are failing because of missing exports.

@paroxysm
Copy link
Copy Markdown
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
Copy Markdown
Contributor

Have you tried starting a relay after those changes?

Comment thread packages/core-logger/package.json Outdated
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.

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@paroxysm
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@paroxysm
Copy link
Copy Markdown
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