-
Notifications
You must be signed in to change notification settings - Fork 4
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
[v2] release prep #137
[v2] release prep #137
Conversation
chore: convert functions to arrow functions
chore: move node_test folder into github folder
Move node tests
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.
LGTM!
No blockers, just a few comments.
@@ -18,10 +18,8 @@ export function composeStrategy( | |||
| FromJSONStrategy | |||
)[] | |||
): FromJSONStrategy | ToJSONStrategy { | |||
return function _composeStrategy( |
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 think the original reason we did this was to that in exceptions we would get a named function to show up. I have no strong opinion one way or the other though.
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.
yeap. @ms1111 mentioned this as well and initially, that was the idea, however, the parent function will display in the stack trace which will lead developers to the right location. There are other benefits as well, we are not creating a new object prototype, which also means it cannot be new
ed and a new function scope is not created.
I think it makes sense that we go with: For global scope, we can use function
, for new
constructors we can use class
and for everything else we can use () => {}
.
!node_tests/package.json | ||
!node_tests/README.md | ||
!node_tests/fixtures | ||
.github/node_tests/* |
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.
What's the reasoning behind this change? In addition to the CI aspect I thought that the node tests were also a code example of how you can use ts_serialize in a typescript context, so it made sense to have them as part of the docs.
On the other hand node usage is covered in the README, so I'm fine with this change
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.
basically from conversations, it was trying to be both examples and tests, but not doing a good job of either. This move is to remove it from the root so the docs/README is used. With the changes to the readme now this is only used on github
so it made sense to move the folder.
TBH, the deno -> node conversation has probably moved forward from when we last looked and there might exist a solution that allows us to run the deno tests within a node env.
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.
Sounds good, I will create a discussion to see if we can find a better way to transpiling into node
Description
Preparation for v2 release.
For reference the full changelog for v2 is:
JSONValue
flexibility for conversions
composeStrategy
insteaddocs/
toREADME.md
and delete folderiso8601Date
=>iso8601Date()
getNewSerializable
fixing compile issuedeno lint --unstable
#121 address lint issuesfromSerializable
strategyserializable.clone(json: JSONObject): this
Once merged we'll need to update the changelog once more with the release date
Things to look at
README.md
,CHANGELOG.md
, etc..)