-
Notifications
You must be signed in to change notification settings - Fork 15
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
Separate typings for node and browser #10
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 3 3
Lines 91 91
Branches 18 18
=====================================
Hits 91 91
Continue to review full report at Codecov.
|
@@ -17,39 +17,40 @@ | |||
], | |||
"main": "node/index.js", | |||
"module": "node/module.js", | |||
"typings": "render.d.ts", | |||
"typings": "src/node.d.ts", |
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 "typings": "src/node.d.ts"
? Why not "typings": "src/index.d.ts"
?
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.
Well, ignore it. src/node.d.ts
makes more sense.
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.
index.{js,d.ts}
- common code for node and browser which exports internalrenderer
andrenderToString
browser.{js,d.ts}
- client-side code which re-exportsrenderToString
from index and exportswithRender
withtoString
onlynode.{js,d.ts}
- server-side code which re-exportsrenderToString
from index plusrenderToStream
and also exportswithRender
withtoString
andtoStream
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.
But I think that App
interface should be in Hyperapp
package. In fact app()
function has the same definition. It makes more sense for me refers from Hyperapp
. Because now you have two App
interfaces (both are the same).
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.
In short, App
is the type for one app()
function that withRender()
will need as param.
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.
index.{js,d.ts} - common code for node and browser which exports internal renderer and renderToString
browser.{js,d.ts} - client-side code which re-exports renderToString from index and exports withRender with toString only
node.{js,d.ts} - server-side code which re-exports renderToString from index plus renderToStream and also exports withRender with toString and toStream
Great!
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.
Only a detail about importing.
Here some examples:
// 1. Testing:
import { renderToStream, withRender, Render, renderToString } from "../../src/node"
// 2. Actually: if I want to import node version
import { renderToStream, withRender, Render, renderToString } from "@hyperapp/render/src/node"
// 3. Purpose: move typings to root directory
import { renderToStream, withRender, Render, renderToString } from "@hyperapp/render/node"
Why do you think about the third option?
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.
Both 2 and 3 options are valid!
They already work in current v2.0.0 and after this PR will work with typings 🎉
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.
fine!
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.
The PR was sent..
Types of changes
Checklist: