Skip to content

Fix compilation and tests failures #8902

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion packages/firestore/rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ const browserPlugins = [
abortOnError: true,
transformers: [util.removeAssertAndPrefixInternalTransformer]
}),
json({ preferConst: true }),
json({ preferConst: true })
//terser(util.manglePrivatePropertiesOptions)
Copy link
Contributor

Choose a reason for hiding this comment

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

needs to be re-enabled

];

Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/rollup.shared.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ exports.removeAssertTransformer = removeAssertTransformer;
*/
const removeAssertAndPrefixInternalTransformer = service => ({
before: [
removeAsserts(service.getProgram()),
removeAsserts(service.getProgram())
// renameInternals(service.getProgram(), {
// publicIdentifiers,
// prefix: '__PRIVATE_'
Copy link
Contributor

Choose a reason for hiding this comment

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

to be re-enabled

Expand Down
34 changes: 9 additions & 25 deletions packages/firestore/src/api/pipeline_impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
firestoreClientExecutePipeline,
firestoreClientListen
} from '../core/firestore_client';
import { ListenerDataSource } from '../core/event_manager';

Check failure on line 30 in packages/firestore/src/api/pipeline_impl.ts

View workflow job for this annotation

GitHub Actions / Lint

`../core/event_manager` import should occur before import of `../core/firestore_client`
import { toCorePipeline } from '../core/pipeline-util';
import { ViewSnapshot } from '../core/view_snapshot';
import { Pipeline as LitePipeline } from '../lite-api/pipeline';
Expand Down Expand Up @@ -87,31 +87,16 @@
* @param pipeline The pipeline to execute.
* @return A Promise representing the asynchronous pipeline execution.
*/
export function execute(pipeline: LitePipeline): Promise<PipelineSnapshot>;
export function execute(
pipeline: RealtimePipeline
): Promise<RealtimePipelineSnapshot>;
export function execute(
pipeline: LitePipeline | RealtimePipeline
): Promise<PipelineSnapshot | RealtimePipelineSnapshot> {
export function execute(pipeline: LitePipeline): Promise<PipelineSnapshot> {
const firestore = cast(pipeline._db, Firestore);
const client = ensureFirestoreConfigured(firestore);

if (pipeline instanceof RealtimePipeline) {
return firestoreClientGetDocumentsViaSnapshotListener(
client,
pipeline
).then(
snapshot =>
new RealtimePipelineSnapshot(pipeline as RealtimePipeline, snapshot)
);
} else {
return firestoreClientExecutePipeline(client, pipeline).then(result => {
// Get the execution time from the first result.
// firestoreClientExecutePipeline returns at least one PipelineStreamElement
// even if the returned document set is empty.
const executionTime =
result.length > 0 ? result[0].executionTime?.toTimestamp() : undefined;
return firestoreClientExecutePipeline(client, pipeline).then(result => {
// Get the execution time from the first result.
// firestoreClientExecutePipeline returns at least one PipelineStreamElement
// even if the returned document set is empty.
const executionTime =
result.length > 0 ? result[0].executionTime?.toTimestamp() : undefined;

const docs = result
// Currently ignore any response from ExecutePipeline that does
Expand All @@ -131,9 +116,8 @@
)
);

return new PipelineSnapshot(pipeline, docs, executionTime);
});
}
return new PipelineSnapshot(pipeline, docs, executionTime);
});
}

// Augment the Firestore class with the pipeline() factory method
Expand Down
1 change: 0 additions & 1 deletion packages/firestore/src/api/snapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import { newQueryComparator } from '../core/query';
import { ChangeType, ViewSnapshot } from '../core/view_snapshot';
import { FieldPath } from '../lite-api/field_path';
import { PipelineResult, toPipelineResult } from '../lite-api/pipeline-result';
import { PipelineResult, toPipelineResult } from '../lite-api/pipeline-result';
import {
DocumentData,
DocumentReference,
Expand Down
36 changes: 1 addition & 35 deletions packages/firestore/src/core/expressions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ export class CoreField implements EvaluableExpr {
});
}
// Return 'UNSET' if the field doesn't exist, otherwise the Value.
const result = input.data.field(this.expr.fieldPath);
const result = input.data.field(this.expr._fieldPath);
if (!!result) {
return EvaluateResult.newValue(result);
} else {
Expand Down Expand Up @@ -367,38 +367,6 @@ export class CoreListOfExprs implements EvaluableExpr {
}
}

export class CoreListOfExprs implements EvaluableExpr {
constructor(private expr: ListOfExprs) {}

evaluate(
context: EvaluationContext,
input: PipelineInputOutput
): EvaluateResult {
return EvaluateResult.newValue(this.expr._getValue());
}
}

export class CoreListOfExprs implements EvaluableExpr {
constructor(private expr: ListOfExprs) {}

evaluate(
context: EvaluationContext,
input: PipelineInputOutput
): EvaluateResult {
const results: EvaluateResult[] = this.expr.exprs.map(expr =>
toEvaluable(expr).evaluate(context, input)
);
// If any sub-expression resulted in error or was unset, the list evaluation fails.
if (results.some(value => value.isErrorOrUnset())) {
return EvaluateResult.newError();
}

return EvaluateResult.newValue({
arrayValue: { values: results.map(value => value.value!) }
});
}
}

function asDouble(
protoNumber:
| { doubleValue: number | string }
Expand Down Expand Up @@ -2576,8 +2544,6 @@ export class CoreUnixMicrosToTimestamp extends UnixToTimestamp {
const nanos = Number((value % MICROSECONDS_PER_SECOND) * BigInt(1000));
return EvaluateResult.newValue({ timestampValue: { seconds, nanos } });
}

abstract toTimestamp(value: bigint): Value | undefined;
}

export class CoreUnixMillisToTimestamp extends UnixToTimestamp {
Expand Down
3 changes: 1 addition & 2 deletions packages/firestore/src/core/pipeline-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
* limitations under the License.
*/

import { RealtimePipeline } from '../api/realtime_pipeline';
import { RealtimePipeline } from '../api/realtime_pipeline';
import { Firestore } from '../lite-api/database';
import {
Expand All @@ -37,7 +36,7 @@ import {
AggregateFunction
} from '../lite-api/expressions';
import { Pipeline, Pipeline as ApiPipeline } from '../lite-api/pipeline';
import { doc } from '../lite-api/reference';
import { doc, DocumentReference } from '../lite-api/reference';
import {
AddFields,
Aggregate,
Expand Down
2 changes: 0 additions & 2 deletions packages/firestore/src/core/sync_engine_impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,6 @@ import { ListenSequence } from './listen_sequence';
import { getPipelineCollectionId, getPipelineSourceType } from './pipeline';
import {
canonifyQueryOrPipeline,
getPipelineCollectionId,
getPipelineSourceType,
isPipeline,
QueryOrPipeline,
queryOrPipelineEqual,
Expand Down
4 changes: 0 additions & 4 deletions packages/firestore/src/lite-api/pipeline_impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
* limitations under the License.
*/

import { RealtimePipeline } from '../api/realtime_pipeline';
import { invokeExecutePipeline } from '../remote/datastore';

import { getDatastore } from './components';
Expand All @@ -28,9 +27,6 @@ import { LiteUserDataWriter } from './reference_impl';
import { Stage } from './stage';
import { newUserDataReader } from './user_data_reader';

// TODO should not be in lite
import { RealtimePipeline} from "../api/realtime_pipeline";

declare module './database' {
interface Firestore {
pipeline(): PipelineSource<Pipeline>;
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/src/lite-api/user_data_reader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -712,7 +712,7 @@ export function parseData(
input = getModularInstance(input);

// Workaround for circular dependency
if ((input as Constant).exprType === 'Constant') {
if ((input as Constant)?.exprType === 'Constant') {
return (input as Constant)._getValue();
}

Expand Down
11 changes: 2 additions & 9 deletions packages/firestore/src/local/local_store_impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,15 @@
import { User } from '../auth/user';
import { BundleConverter, BundledDocuments, NamedQuery } from '../core/bundle';
import { CorePipeline, getPipelineDocuments } from '../core/pipeline';

import {
canonifyTargetOrPipeline,
isPipeline,
QueryOrPipeline,
TargetOrPipeline,
targetOrPipelineEqual
} from '../core/pipeline-util';
import {
canonifyTargetOrPipeline,
getPipelineDocuments,
isPipeline,
QueryOrPipeline,
TargetOrPipeline,
targetOrPipelineEqual
} from '../core/pipeline-util';
import { CorePipeline } from '../core/pipeline_run';

import {
newQueryForPath,
queryCollectionGroup,
Expand Down
7 changes: 2 additions & 5 deletions packages/firestore/test/integration/api/query.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,7 @@ import {
withTestCollection,
withTestDb
} from '../util/helpers';
import {
onSnapshot as onPipelineSnapshot,
execute
} from '../util/pipeline_export';
import { onSnapshot as onPipelineSnapshot } from '../util/pipeline_export';
import { USE_EMULATOR } from '../util/settings';
import { captureExistenceFilterMismatches } from '../util/testing_hooks_util';

Expand Down Expand Up @@ -1458,7 +1455,7 @@ apiPipelineDescribe.only('Queries', (persistence, pipelineMode) => {
});
});

it.only('can use filter with nested field', () => {
it('can use filter with nested field', () => {
// Reproduces https://github.com/firebase/firebase-js-sdk/issues/2204
const testDocs = {
a: {},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
doc,
DocumentSnapshot,
enableNetwork,
Firestore,
getDoc,
limit,
limitToLast,
Expand Down Expand Up @@ -91,7 +92,9 @@ apiPipelineDescribe.only(
new EventsAccumulator<RealtimePipelineSnapshot>();
const unsubscribe = onSnapshot(
pipelineMode,
docRef.firestore.realtimePipeline().documents([docRef]),
(docRef.firestore as Firestore)
.realtimePipeline()
.documents([docRef]),
{ source: 'cache' },
storeEvent.storeEvent
);
Expand Down
34 changes: 26 additions & 8 deletions packages/firestore/test/integration/util/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ import {
TARGET_DB_ID,
USE_EMULATOR
} from './settings';
import { _onRealtimePipelineSnapshot } from '../../../src/api/pipeline_impl';
import { RealtimePipeline } from '../../../src/api/realtime_pipeline';
import { onPipelineSnapshot } from '../../../src/api/reference_impl';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MarkDuckworth This import is suspicious, just to call it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll take another look.


/* eslint-disable no-restricted-globals */

Expand Down Expand Up @@ -647,9 +647,11 @@ export async function checkOnlineAndOfflineResultsMatchWithPipelineMode(
await checkOnlineAndOfflineResultsMatch(query, ...expectedDocs);
} else {
// pipelineMode === 'query-to-pipeline'
const pipeline = query.firestore.realtimePipeline().createFrom(query);
const pipeline = (query.firestore as Firestore)
.realtimePipeline()
.createFrom(query);
const deferred = new Deferred<RealtimePipelineSnapshot>();
const unsub = _onRealtimePipelineSnapshot(
const unsub = onPipelineSnapshot(
pipeline,
{ includeMetadataChanges: true },
snapshot => {
Expand All @@ -668,7 +670,7 @@ export async function checkOnlineAndOfflineResultsMatchWithPipelineMode(
}

const cacheDeferred = new Deferred<RealtimePipelineSnapshot>();
const cacheUnsub = _onRealtimePipelineSnapshot(
const cacheUnsub = onPipelineSnapshot(
pipeline,
{ includeMetadataChanges: true, source: 'cache' },
snapshot => {
Expand All @@ -689,6 +691,22 @@ export function itIf(
return condition === 'only' ? it.only : condition ? it : it.skip;
}

function getDocsFromPipeline(
pipeline: RealtimePipeline
): Promise<RealtimePipelineSnapshot> {
const deferred = new Deferred<RealtimePipelineSnapshot>();
const unsub = onSnapshot(
'query-to-pipeline',
pipeline,
(snapshot: RealtimePipelineSnapshot) => {
deferred.resolve(snapshot);
unsub();
}
);

return deferred.promise;
}

export function getDocs(
pipelineMode: PipelineMode,
queryOrPipeline: Query | RealtimePipeline
Expand All @@ -698,7 +716,7 @@ export function getDocs(
const ppl = queryOrPipeline.firestore
.pipeline()
.createFrom(queryOrPipeline);
return getDocsProd(
return getDocsFromPipeline(
new RealtimePipeline(
ppl._db,
ppl.userDataReader,
Expand All @@ -707,7 +725,7 @@ export function getDocs(
)
);
} else {
return getDocsProd(queryOrPipeline);
return getDocsFromPipeline(queryOrPipeline);
}
}

Expand Down Expand Up @@ -743,7 +761,7 @@ export function onSnapshot(
const ppl = queryOrPipeline.firestore
.pipeline()
.createFrom(queryOrPipeline);
return onSnapshotProd(
return onPipelineSnapshot(
new RealtimePipeline(
ppl._db,
ppl.userDataReader,
Expand All @@ -754,7 +772,7 @@ export function onSnapshot(
obs as any
);
} else {
return onSnapshotProd(queryOrPipeline, options as any, obs as any);
return onPipelineSnapshot(queryOrPipeline, options as any, obs as any);
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/test/unit/specs/spec_builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import {
TargetOrPipeline,
targetOrPipelineEqual,
toCorePipeline,
toPipeline
toPipelineStages
} from '../../../src/core/pipeline-util';
import {
LimitType,
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/test/unit/specs/spec_test_runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ import {
queryOrPipelineEqual,
TargetOrPipeline,
toCorePipeline,
toPipeline
toPipelineStages
} from '../../../src/core/pipeline-util';
import {
canonifyQuery,
Expand Down
Loading