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

fix(loader-utils): ReadableFile implementation to match the interface #3157

Merged
merged 2 commits into from
Nov 8, 2024
Merged

fix(loader-utils): ReadableFile implementation to match the interface #3157

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb2bac3
ReadableFile implementation to match the interface
dariaterekhova-actionengine Nov 5, 2024
7bfa377
Review corrections
dariaterekhova-actionengine Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions examples-experimental/slpk-in-browser/src/app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@ import {MapController, FlyToInterpolator, MapViewState} from '@deck.gl/core/type
import {COORDINATE_SYSTEM, I3SLoader, parseSLPKArchive} from '@loaders.gl/i3s';
import {Tileset3D} from '@loaders.gl/tiles';
import {ControlPanel} from './components/control-panel';
import {BrowserFile} from './browser-file';
import {ZipFileSystem} from '@loaders.gl/zip';
import {LoaderWithParser} from '@loaders.gl/loader-utils';
import {BlobFile, FileProvider, LoaderWithParser} from '@loaders.gl/loader-utils';
import CustomTile3DLayer from './custom-tile-3d-layer';

export const TRANSITION_DURAITON = 4000;
Expand Down Expand Up @@ -49,7 +48,7 @@ export default function App() {
}

const createFileSystem = async (file: File) => {
const fileProvider = new BrowserFile(file);
const fileProvider = await FileProvider.create(new BlobFile(file));
const archive = await parseSLPKArchive(fileProvider, undefined, file.name);
const fileSystem = new ZipFileSystem(archive);
setFileSystem(fileSystem);
Expand Down
132 changes: 0 additions & 132 deletions examples-experimental/slpk-in-browser/src/browser-file.ts
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion examples-experimental/slpk-in-browser/vite.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const getAliases = async (frameworkName, frameworkRootDir) => {

// https://vitejs.dev/config/
export default defineConfig(async () => ({
resolve: {alias: await getAliases('@loaders.gl', `${__dirname}/../../..`)},
resolve: {alias: await getAliases('@loaders.gl', `${__dirname}/../..`)},
server: {open: true}
}))

6 changes: 4 additions & 2 deletions modules/loader-utils/src/lib/files/blob-file.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ export class BlobFile implements ReadableFile {
};
}

async read(start: number, length: number): Promise<ArrayBuffer> {
const arrayBuffer = await this.handle.slice(start, start + length).arrayBuffer();
async read(start?: number | BigInt, length?: number): Promise<ArrayBuffer> {
const arrayBuffer = await this.handle
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I faced a problem when I have ReadableFile as input in FileProvider and read() there accepts start as number | bigint so I pass bigint here and get runtime error that it cannot sum bigint and number because real implementation knows nothing about bigint. We need keep real implementation of the methods matching it's description in ReadableFile

.slice(Number(start), Number(start) + Number(length))
.arrayBuffer();
return arrayBuffer;
}
}
4 changes: 2 additions & 2 deletions modules/loader-utils/src/lib/files/http-file.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ export class HttpFile implements ReadableFile {
};
}

async read(offset: number | bigint, length: number): Promise<ArrayBuffer> {
const response = await this.fetchRange(offset, length);
async read(offset?: number | bigint, length?: number): Promise<ArrayBuffer> {
const response = await this.fetchRange(offset ?? 0, length ?? 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use default argument syntax for better clarity?

Suggested change
async read(offset?: number | bigint, length?: number): Promise<ArrayBuffer> {
const response = await this.fetchRange(offset ?? 0, length ?? 0);
async read(offset: number | bigint = 0, length: number = 0): Promise<ArrayBuffer> {
const response = await this.fetchRange(offset, length);

const arrayBuffer = await response.arrayBuffer();
return arrayBuffer;
}
Expand Down
2 changes: 1 addition & 1 deletion modules/loader-utils/src/lib/files/node-file-facade.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export class NodeFileFacade implements ReadableFile, WritableFile {
throw new Error('Can\'t instantiate NodeFile. Make sure to import @loaders.gl/polyfills first.');
}
/** Read data */
async read(start?: number | bigint, end?: number | bigint): Promise<ArrayBuffer> {
async read(start?: number | bigint, offset?: number): Promise<ArrayBuffer> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • What is the difference between start and offset?
  • How does this function now know where to stop reading?
  • Should it be length instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's my mistake, I'll change it to length, as it's in the ReadableFile and NodeFile implementation

throw NOT_IMPLEMENTED;
}
/** Write to file. The number of bytes written will be returned */
Expand Down
Loading