-
Notifications
You must be signed in to change notification settings - Fork 286
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
Package up core-logger types #1833
Conversation
Tests are failing because of missing exports. |
Tests are now passing, the issue was that I hadn't included |
Have you tried starting a relay after those changes? |
packages/core-logger/package.json
Outdated
@@ -6,7 +6,8 @@ | |||
"Brian Faust <brian@ark.io>" | |||
], | |||
"license": "MIT", | |||
"main": "dist/index.js", | |||
"main": "index", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Looks ok for now, ultimately I would aim for proper declaration files though. |
@faustbrian Based on the typescript compiler options that we're inheriting from |
Feel free to send PRs for other packages, the foreseeable future that solution is good enough. |
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! |
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
Checklist