Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update: Changing on Dependencies #4

Closed
wants to merge 8 commits into from
Closed

Update: Changing on Dependencies #4

wants to merge 8 commits into from

Conversation

UtkuErdemir
Copy link
Contributor

@aaydin-tr please check this.

aaydin-tr
aaydin-tr reviewed Dec 31, 2023
View reviewed changes
package.json Outdated
Comment on lines 54 to 58
"@typescript-eslint/eslint-plugin": "^6.11.0",
"@typescript-eslint/parser": "^6.11.0",
"bun-types": "latest",
"eslint": "^8.53.0",
"@typescript-eslint/eslint-plugin": "^6.16.0",
"@typescript-eslint/parser": "^6.16.0",
"bun-types": "^1.0.20",
"eslint": "^8.55.0",
"eslint-plugin-import": "^2.29.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we are changing dependencies is there any specific reason

Copy link
Contributor Author

@UtkuErdemir UtkuErdemir Dec 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just staying up to date. When we run bun update, it updates directly. Just i have removed latest tag from there because in the future it may create an issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess latest can be stay for bun-types

package.json Outdated
@@ -63,6 +63,6 @@
"dependencies": {
"fast-querystring": "^1.1.2",
"radix3": "^1.1.0",
"reflect-metadata": "^0.1.13"
"reflect-metadata": "^0.2.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we are changing dependencies is there any specific reason

src/context.ts Outdated
...this.res.headers.toJSON(),
...Object.fromEntries(this.res.headers),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toJSON() in bun about 10x faster than Object.fromEntries(headers.entries())

Copy link
Contributor Author

@UtkuErdemir UtkuErdemir Dec 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be, but that usage fails pipeline for TS checking. I didn't prefer that, i have changed when pipeline was failed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting should't be fail can you rollback toJSON and retry

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added back and pipeline is failed again. Could you look that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just rollback all toJSON i can force pipeline it should't be fail

src/context.ts
Comment on lines -324 to 325
...this.res.headers.toJSON(),
...Object.fromEntries(this.res.headers),
"Content-Type": "application/json",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same above

src/context.ts
headers: this.res.headers,
headers: Object.fromEntries(this.res.headers),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same above

src/context.ts
...this.res.headers.toJSON(),
...Object.fromEntries(this.res.headers),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same above

src/context.ts
...this.res.headers.toJSON(),
...Object.fromEntries(this.res.headers),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same above

src/context.ts
...this.res.headers.toJSON(),
...Object.fromEntries(this.res.headers),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same above

@aaydin-tr
Copy link
Contributor

tsc give error because update i guess best thing to do close this mr and update dependencies one by one what do you say @UtkuErdemir

@aaydin-tr
Copy link
Contributor

aaydin-tr commented Dec 31, 2023 •
edited
Loading

actually no need to update devDependencies or peerDependencies right now bun we can update reflect-metadata

@UtkuErdemir
Copy link
Contributor Author

@aaydin-tr i have reverted package.json and bunlock but it changed nothing.

@aaydin-tr
Copy link
Contributor

closing this mr because no need to change our dependencies right now and its break pipeline

@aaydin-tr aaydin-tr closed this Dec 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaydin-tr aaydin-tr aaydin-tr left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@UtkuErdemir @aaydin-tr