-
Notifications
You must be signed in to change notification settings - Fork 14
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
Introduce rollup to bundle the package #73
Conversation
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.
Looks good to me, I can double check the output at work tomorrow but from reading here it should be fine. Thanks for raising the PR!
Thanks, that's quick! I'll try to review and try it out in a few hours. If everything is fine I'll release 4.0.1 and deprecate 4.0.0. |
Oh, I realised that I was unclear. I'll hold again, and await the testing by @SimeonC before releasing. |
Looks good to me! |
@@ -2,3 +2,7 @@ import { Factory } from './factory'; | |||
|
|||
export default new Factory().newBuilder(); | |||
export type { JUnitReportBuilder as Builder } from './builder'; | |||
export type { TestSuite } from './test_suite'; | |||
export type { TestCase } from './test_case'; | |||
export type { TestGroup } from './test_group'; |
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.
If I recall correctly, TestGroup
could/should be abstract
, and if so it should not be exported here. But I'll confirm and change that myself.
@@ -1,13 +1,11 @@ | |||
{ | |||
"compilerOptions": { | |||
"target": "es2018", | |||
"module": "commonjs", | |||
"module": "ESNext", |
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.
Is ESNext
what Rollup expects, or does that introduce any constraints on supported node.js versions? Or does it not matter, as long as the TypeScript source files don't use any shiny new features?
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 is what rollup needs.
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.
Target allows to "dumb down" features.
package.json
Outdated
@@ -70,9 +70,7 @@ | |||
} | |||
}, | |||
"files": [ | |||
"./dist/*", | |||
"README.md" |
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.
Why is this not needed? Is this included anyway?
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.
Readme is not needed, dist is.
Brilliant, thanks again! I'll run some more tests, update the readme a bit more, and then get a new release out. |
I've tested it locally with maplibre-gl-js project and it seems to work as expected.
Let me know if there are more changes that need to be done.
Here's the generated package if you want to try it out locally.
junit-report-builder-4.0.0.zip.zip