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 node to v20 lts #1688

Closed
wants to merge 10 commits into from
Closed

update node to v20 lts #1688

wants to merge 10 commits into from

Conversation

akellbl4
Copy link
Collaborator

@akellbl4 akellbl4 force-pushed the ak/update-node branch 2 times, most recently from c98f914 to 3064792 Compare October 28, 2023 06:53
@codecov
Copy link

codecov bot commented Oct 28, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (82c6178) 61.47% compared to head (6917ad0) 61.47%.
Report is 1 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #1688   +/-   ##
=======================================
  Coverage   61.47%   61.47%           
=======================================
  Files         132      132           
  Lines        2998     2998           
  Branches      757      757           
=======================================
  Hits         1843     1843           
  Misses       1038     1038           
  Partials      117      117

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link

github-actions bot commented Oct 28, 2023 •
edited
Loading

size-limit report 📦

Path Size
public/embed.mjs 2.03 KB (0%)
public/remark.mjs 73.45 KB (-0.01% 🔽)
public/remark.css 8.26 KB (-0.04% 🔽)
public/last-comments.mjs 35.72 KB (-0.01% 🔽)
public/last-comments.css 3.75 KB (0%)
public/deleteme.mjs 12.07 KB (0%)
public/counter.mjs 751 B (0%)

@akellbl4 akellbl4 force-pushed the ak/update-node branch 4 times, most recently from 97b37c3 to 7c754ab Compare October 28, 2023 16:15
Mavrin
Mavrin requested changes Nov 2, 2023
View reviewed changes
frontend/packages/api/lib/fetcher.ts Outdated Show resolved Hide resolved
frontend/packages/api/tests/clients/admin.test.ts Outdated
@@ -6,7 +6,7 @@ interface Context {
client: ReturnType<typeof createAdminClient>
}

describe<Context>('Admin Client', (adminClient) => {
describe.skip<Context>('Admin Client', (adminClient) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you skip this test

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reworked the whole thing, tests are fixed.

frontend/e2e/screenshot-firefox.png Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

why does firefox show only one comment? I see that other browsers render 5 comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ops, those shouldn't get into the repo.
Those were created while local testing and FF had some problems.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 12, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 7623478702

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 84.833%
Totals Coverage Status
Change from base Build 7596391691: 0.06%
Covered Lines: 5968
Relevant Lines: 7035
💛 - Coveralls

@akellbl4 akellbl4 requested a review from Mavrin November 12, 2023 05:54
Mavrin
Mavrin requested changes Jan 17, 2024
View reviewed changes
frontend/packages/api/package.json
".": {
"import": "./dist/index.js",
"require": "./dist/index.js",
"types": "./dist/index.d.ts"
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://nodejs.org/api/packages.html#community-conditions-definitions

"types" - can be used by typing systems to resolve the typing file for the given export. This condition should always be included first.

frontend/packages/api/src/lib/fetcher.ts
// overrides type of init in URLSearchParams constructor because it's not correct and accepts QueryParams
const searchParams = new URLSearchParams(
Object.entries(query).reduce<Record<string, string>>(
(acc, [k, v]) => Object.assign(acc, { [k]: v.toString() }),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't need to create new object on every iterations and we can mutate acc

acc[k] = v.toString();

@akellbl4 akellbl4 closed this Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mavrin Mavrin Mavrin requested changes

@umputun umputun Awaiting requested review from umputun umputun is a code owner

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

Successfully merging this pull request may close these issues.
2 participants
@akellbl4 @Mavrin