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.

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

fix: Node imports paths #1141

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Node imports paths #1141

wants to merge 1 commit into from

Conversation

skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented Dec 10, 2024

Might fix #1127

  • Always import type when necessary (not the full declaration)
  • always import from the real ts file, not from lib/index.ts

Signed-off-by: skjnldsv <[email protected]>
Copy link

codecov bot commented Dec 10, 2024

Bundle Report

Changes will increase total bundle size by 1.92kB (0.39%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
@nextcloud/files-esm 122.84kB 1.92kB (1.58%) ⬆️

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.62%. Comparing base (0d773f3) to head (8e751c9).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
lib/utils/fileSorting.ts 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1141   +/-   ##
=======================================
  Coverage   91.62%   91.62%           
=======================================
  Files          23       23           
  Lines         633      633           
  Branches      166      166           
=======================================
  Hits          580      580           
  Misses         46       46           
  Partials        7        7           

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

@skjnldsv skjnldsv self-assigned this Dec 10, 2024
@skjnldsv skjnldsv added bug Something isn't working 3. to review labels Dec 10, 2024
@@ -73,7 +73,7 @@ export interface IFileListFilter extends TypedEventTarget<IFileListFilterEvents>
* @param nodes Nodes to filter
* @return Subset of the `nodes` parameter to show
*/
filter(nodes: INode[]): INode[]
filter(nodes: Node[]): Node[]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change and I do not see how it would help.
(I requires the exact implementation with privat methods instead of the only required interface)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall we use INode everywhere then? I find the multiple types confusing.
Which are we supposed to use now ?

import { Folder } from './files/folder.ts'
import { View } from './navigation/view.ts'
import logger from './utils/logger.ts'
import type { Node } from './files/node'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why removing the extensions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consistency

import { CancelablePromise } from 'cancelable-promise'
import { createClient, getPatcher } from 'webdav'
import { generateRemoteUrl } from '@nextcloud/router'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why sorting down? @... should be before c...

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 sort the lines, not the libraries, have been doing this everywhere here for... Years 😂
Funny you're only noticing this now 🤭

@@ -42,7 +42,7 @@ export interface FilesSortingOptions {
* @param nodes Nodes to sort
* @param options Sorting options
*/
export function sortNodes(nodes: readonly INode[], options: FilesSortingOptions = {}): INode[] {
export function sortNodes(nodes: readonly Node[], options: FilesSortingOptions = {}): Node[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, breaking and not needed.
We only need the interface (public) not the private implementation.

@@ -7,7 +7,7 @@
import type { IFileListFilter, Navigation } from './lib'
import type { DavProperty } from './lib/dav/davProperties'
import type { FileAction } from './lib/fileAction'
import type { FileListAction } from './lib/fileListAction.ts'
import type { FileListAction } from './lib/fileListAction'
Copy link
Contributor

Choose a reason for hiding this comment

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

Extension?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing breaking bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redeclaration of Node_2 class causes type incompatibilty
2 participants