-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(nextjs): Add server-side request transactions #3533
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
Merged
lobsterkatie
merged 7 commits into
kmclb-next-js-performance
from
kmclb-next-js-add-request-transactions
May 17, 2021
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b79ee09
page transactions working, API transactions not working
lobsterkatie d5293da
inject SDK init earlier - API transactions working
lobsterkatie de075a4
fix requiring server init code
lobsterkatie c9f78ce
improve documentation
lobsterkatie f352ff2
move tracing package to regular dependencies
lobsterkatie 0ec15fd
fix deepReadDirSync
lobsterkatie 188c982
refactor setting env vars
lobsterkatie File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 this should work as well, but you'd have to confirm whether it allows for the same property-based
for in/of
as[key: string]
does. If so, thenPlainObject
can be removed entirely.Uh oh!
There was an error while loading. Please reload this page.
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 thought I'd learned that
Record
is really meant to be used on a known, finite set of keys. (I get that it would work - I'm talking about should.) Is that not true?I made
PlainObject
just because I was tired of typing{[key: string] : whatever}
a million times, and figured since there'sArray
their might as well be an equivalent for objects. ¯_(ツ)_/¯ I'm not committed to it.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.
Nope, it's basically what you wrote up there with key/value.
https://www.typescriptlang.org/play?ssl=12&ssc=1&pln=13&pc=1#code/C4TwDgpgBAYg9nKBeKAlCBjOAnAJgHgGdhsBLAOwHMAaKcgVwFsAjCbAPgG4Aobrc4lABmCAFywEyKAG9uUKAAtS4gIzU5UYArYRxAJnXzccSpTjiAzNwC+vEXAB0GAIbBgiFABYe9gNoByY0oIfwBdKQBWHm4RbCgACn5BAGsIECgKYQQAShkNJLgAGwgHQpN41JBaP0rQ7JteJOAoSql-JX8eAuLS8pq0us4gA
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.
Yeah, no, I know it works. I just meant idiomatically. Here are a few things I dug out of my bookmarks:
https://stackoverflow.com/a/54100359/3874926
https://fnune.com/typescript/2019/01/30/typescript-series-1-record-is-usually-not-the-best-choice
https://rickcarlino.com/2017/02/27/real-world-use-case-for-typescript-record-types.html
https://stackoverflow.com/questions/51936369/what-is-the-record-type-in-typescript#comment103989203_51937036
Happy to go back to using
{ [key: string]: string }
if you think it's better thanPlainObject
.UPDATE: The playground hates it, but check out what compiles just fine if you actually use it in your code:
I get that it's a little unorthodox, but what do you think of using that?? 😁