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

Cannot freeze array buffer views with elements #4645

Closed
2 tasks
enrico-bellanti opened this issue Dec 27, 2024 · 8 comments
Closed
2 tasks

Cannot freeze array buffer views with elements #4645

enrico-bellanti opened this issue Dec 27, 2024 · 8 comments

Comments

@enrico-bellanti
Copy link

enrico-bellanti commented Dec 27, 2024

Which @ngrx/* package(s) are the source of the bug?

signals

Minimal reproduction of the bug/regression with instructions

This error occurs because @ngrx/signals is trying to deeply freeze array buffer views in your map layers, which isn't supported. The issue is in the state management of OpenLayers map and layer objects.

map.component.ts:45 Error initializing map: TypeError: Cannot freeze array buffer views with elements
at Function.freeze ()
at deepFreeze (ngrx-signals.mjs:82:10)
at ngrx-signals.mjs:92:9
at Array.forEach ()
at deepFreeze (ngrx-signals.mjs:84:38)
at ngrx-signals.mjs:92:9
at Array.forEach ()
at deepFreeze (ngrx-signals.mjs:84:38)
at ngrx-signals.mjs:92:9
at Array.forEach ()

Expected behavior

there are lots of libraries which use ArrayBufferViews (not only open layers as example) and now updating NGRX signals to 19.0.0 i have lots of bugs come out, which can not store object which cannot be deepFrozen, this won't be happened with version 18.0.0

what is the best way to solve these kind of problems avoiding to entire refactor of the app which use @ngrx/signals?

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s)

ngrx/signals 19.0.0
angular 19.0.0

Other information

No response

I would be willing to submit a PR to fix this issue

  • Yes
  • No
@timdeschryver
Copy link
Member

This is probably a duplicate of #4635
Please provide a minimal reproduction or this will be closed.

@rainerhahnekamp
Copy link
Contributor

@timdeschryver, I think that this could be a little bit different. Whereas #4635 is using data that shouldn't be in the state, this might be part of the state.

@enrico-bellanti, as Tim said, we would require a short code snippet.

@enrico-bellanti
Copy link
Author

enrico-bellanti commented Dec 29, 2024

This is the map.store.ts which throws the error:

`
import { computed, inject } from '@angular/core';
import { patchState, signalStore, withComputed, withMethods, withState } from '@ngrx/signals';
import { Map } from 'ol';
import GeoJSON from 'ol/format/GeoJSON';
import Layer from 'ol/layer/Layer';
import { Style } from 'ol/style';
import OLCesium from 'olcs';
import { map, Observable } from 'rxjs';
import { LayerConfig } from '../interfaces/layer.interface';
import { MapConfig, MapTool } from '../interfaces/map.interface';
import { EnhancedLayer } from '../models/EnhancedLayer';
import { LayerUploadService } from '../services/layer-upload.service';
import { MapService } from '../services/map.service';

export interface MapState {
mapRef: string | null;
map: Map | null,
oLCesium: OLCesium | null,
config: MapConfig | null;
layers: EnhancedLayer[];
tools: MapTool[];
is3DEnabled: boolean,
activeDrawingWidgetRef: string | null;
}

const initialState: MapState = {
mapRef: null,
map: null,
oLCesium: null,
config: null,
layers: [],
tools: [],
is3DEnabled: false,
activeDrawingWidgetRef: null
};

export const MapStore = signalStore(
withState(initialState),
withComputed(({ layers, activeDrawingWidgetRef }) => ({
baseLayers: computed(() => layers().filter(l => l.get('isBase') === true)),
publicLayers: computed(() => layers().filter(l => !l.get('private'))),
selectedLayers: computed(() => layers().filter(l => l.get('isSelected'))),
filteredLayers: computed(() => layers().filter(l => l.hasFilters())),
isDrawingActive: computed(() => activeDrawingWidgetRef() !== null),
})),
withMethods((
store,
mapService = inject(MapService),
layerUploadService = inject(LayerUploadService)
) => {
const geoJsonFormat = new GeoJSON();

    const findLayerById = (layerId: string): EnhancedLayer | undefined => {
        return store.publicLayers().find(layer => layer.get('id') === layerId);
    };

    const emitLayerChanges = () => {
        const updatedLayers = [...store.layers()];
        patchState(store, { layers: updatedLayers });
    };

    const updateLayerIfExists = (
        layerId: string,
        updateFn: (layer: EnhancedLayer) => void
    ): boolean => {
        const layer = findLayerById(layerId);
        if (layer) {
            updateFn(layer);
            emitLayerChanges();
            return true;
        }
        return false;
    };

    return {
        initMap(mapRef: string, config?: MapConfig): Observable<void> {
            return mapService.buildMap(mapRef, config).pipe(
                map(({ map, oLCesium }) => {
                    const enhancedLayers = map.getAllLayers().map(l => new EnhancedLayer(l));
                    const baseLayers = enhancedLayers.filter(l => l.get('isBase') === true);

                    if (baseLayers.length > 0) {
                        // Keep first base layer visible, hide the rest
                        baseLayers.forEach((layer, index) => {
                            if (index > 0) {
                                layer.setVisible(false);
                                layer.setPrivate(true)
                            }
                        });
                    }

                    patchState(store, {
                        mapRef,
                        config,
                        map,
                        oLCesium,
                        layers: enhancedLayers
                    });

                    // Subscribe to layer collection changes
                    map.getLayers().on('add', () => {
                        const updatedLayers = map.getAllLayers().map(l => new EnhancedLayer(l));

                        patchState(store, { layers: updatedLayers });
                    });

                    map.getLayers().on('remove', () => {
                        const updatedLayers = map.getAllLayers().map(l => new EnhancedLayer(l));
                        patchState(store, { layers: updatedLayers });
                    });
                })
            );
        },

        updateLayers(layers: Layer[]) {
            const enhancedLayers = layers.map(l => new EnhancedLayer(l));
            patchState(store, { layers: enhancedLayers });
        },

        switchBaseLayer(selectedLayer: EnhancedLayer): void {
            const baseLayers = store.baseLayers();

            // Toggle visibility and privacy for all base layers
            baseLayers.forEach(layer => {
                if (layer === selectedLayer) {
                    // Selected layer should be visible and public
                    layer.setVisible(true);
                    layer.setPrivate(false);
                } else {
                    // Other base layers should be hidden and private
                    layer.setVisible(false);
                    layer.setPrivate(true);
                }
            });

            emitLayerChanges();
        },

        clearLayerFilters(layerId: string) {
            const layer = findLayerById(layerId);
            if (!layer) return;

            layer.clearFilters();
            emitLayerChanges();
        },

        toggleLayerSelection(layerId: string): boolean {
            return updateLayerIfExists(layerId, (layer) => {
                const isLayerSelected = layer.get('isSelected') as boolean | undefined;
                layer.set('isSelected', !isLayerSelected);
            });
        },

        // 3D mode controls
        toggle3DMode() {
            const oLCesium = store.oLCesium();
            if (oLCesium) {
                const currentState = !store.is3DEnabled();
                oLCesium.setEnabled(currentState);
                patchState(store, { is3DEnabled: currentState });
            }
        },

        set3DMode(is3DEnabled: boolean) {
            const oLCesium = store.oLCesium();
            if (oLCesium) {
                oLCesium.setEnabled(is3DEnabled);
                patchState(store, { is3DEnabled });
            }
        },
        // Drawing coordination methods
        registerDrawingWidget(widgetRef: string): boolean {
            if (store.activeDrawingWidgetRef() === null) {
                patchState(store, { activeDrawingWidgetRef: widgetRef });
                return true;
            }
            return false;
        },

        unregisterDrawingWidget(widgetRef: string): void {
            if (store.activeDrawingWidgetRef() === widgetRef) {
                patchState(store, { activeDrawingWidgetRef: null });
            }
        },

        isDrawingAllowed(widgetRef: string): boolean {
            return store.activeDrawingWidgetRef() === null ||
                store.activeDrawingWidgetRef() === widgetRef;
        },
        addLayerFromConfig(config: LayerConfig): Observable<void> {
            return layerUploadService.createLayerFromConfig(config).pipe(
                map(enhancedLayer => {
                    const map = store.map();
                    if (map) {
                        if (config.isBase) {
                            map.getLayers().insertAt(0, enhancedLayer.layer);
                        } else {
                            map.addLayer(enhancedLayer.layer);
                        }
                    }
                })
            );
        },

        addLayerFromFile(file: File, options?: {
            name?: string;
            style?: Style;
            visible?: boolean;
            opacity?: number;
        }): Observable<void> {
            return layerUploadService.createLayerFromFile(file, options).pipe(
                map(enhancedLayer => {
                    const map = store.map();
                    if (map) {
                        map.addLayer(enhancedLayer.layer);
                    }
                })
            );
        },

        addLayerFromUrl(url: string, options?: {
            name?: string;
            style?: Style;
            visible?: boolean;
            opacity?: number;
        }): Observable<void> {
            return layerUploadService.createLayerFromUrl(url, options).pipe(
                map(enhancedLayer => {
                    const map = store.map();
                    if (map) {
                        map.addLayer(enhancedLayer.layer);
                    }
                })
            );
        }
    }
})

);`

this is the part which throws the error

        initMap(mapRef: string, config?: MapConfig): Observable<void> {
            return mapService.buildMap(mapRef, config).pipe(
                map(({ map, oLCesium }) => {
                    const enhancedLayers = map.getAllLayers().map(l => new EnhancedLayer(l));
                    const baseLayers = enhancedLayers.filter(l => l.get('isBase') === true);

                    if (baseLayers.length > 0) {
                        // Keep first base layer visible, hide the rest
                        baseLayers.forEach((layer, index) => {
                            if (index > 0) {
                                layer.setVisible(false);
                                layer.setPrivate(true)
                            }
                        });
                    }

                    patchState(store, {
                        mapRef,
                        config,
                        map,
                        oLCesium,
                        layers: enhancedLayers
                    });

                    // Subscribe to layer collection changes
                    map.getLayers().on('add', () => {
                        const updatedLayers = map.getAllLayers().map(l => new EnhancedLayer(l));

                        patchState(store, { layers: updatedLayers });
                    });

                    map.getLayers().on('remove', () => {
                        const updatedLayers = map.getAllLayers().map(l => new EnhancedLayer(l));
                        patchState(store, { layers: updatedLayers });
                    });
                })
            );
        },

do you suggest a different approach to achieve this in ngrx 19.0.0?

@rainerhahnekamp
Copy link
Contributor

The quick fix would be to opt out Object.freeze when we encounter unfreezable types.


The more important question is if these objects should actually end up in the state.

You see, the concept of having an instance of a class is not really compatible with state that requires immutable changes. You would have to create a new instance whenever you want to change the value of a property.

For example,

layer.setVisible(false);
layer.setPrivate(true)

layer stays the same object reference. so no one computed or effect would be notified about that change.

or

set3DMode(is3DEnabled: boolean) {
  const oLCesium = store.oLCesium();
  if (oLCesium) {
    // the next two lines just don't like right to me
    oLCesium.setEnabled(is3DEnabled);
    patchState(store, { is3DEnabled });
  }
},

That's not how it should be. And it is not really related to State Management, that's an issue you have with normal Signals as well.

@enrico-bellanti
Copy link
Author

enrico-bellanti commented Dec 30, 2024

The quick fix would be to opt out Object.freeze when we encounter unfreezable types.

The more important question is if these objects should actually end up in the state.

You see, the concept of having an instance of a class is not really compatible with state that requires immutable changes. You would have to create a new instance whenever you want to change the value of a property.

For example,

layer.setVisible(false);
layer.setPrivate(true)

layer stays the same object reference. so no one computed or effect would be notified about that change.

or

set3DMode(is3DEnabled: boolean) {
  const oLCesium = store.oLCesium();
  if (oLCesium) {
    // the next two lines just don't like right to me
    oLCesium.setEnabled(is3DEnabled);
    patchState(store, { is3DEnabled });
  }
},

That's not how it should be. And it is not really related to State Management, that's an issue you have with normal Signals as well.

So what do you suggest to be available that properties around the app ? with a service?

and how to avoid Object.freeze of encounter unfreezable type in that state?

@rainerhahnekamp
Copy link
Contributor

and how to avoid Object.freeze of encounter unfreezable type in that state?

The quick fix needs to come from us.

So what do you suggest to be available that properties around the app ? with a service?

I see OpenLayers for the first time. Whereas the SignalStore is optimized for Angular Signals, OpenLayers doesn't use Signals at all. And if you force them into Signals, it might not be optimal.

What could work, though, is to use the SignalStore for the state without OpenLayer types and then use withComputed to derive the map and layer objects. But that depends on your application (which I don't know so well 😅).

@rainerhahnekamp
Copy link
Contributor

We have two options:

  1. Catch failing freeze operations
    Display a warning in the console if a freeze operation fails.

  2. Provide a configuration option
    Allow disabling freezing for a SignalStore class through a configuration
    value: signalStore({ strictStateImmutability: false })

I prefer the configuration approach, as it aligns with how this is handled in
the global store.

@markostanimirovic
Copy link
Member

SignalStore's state has to be immutable.

Mutable objects with methods are state containers themselves. They should not be defined as SignalStore state slices. Instead, you can define them as SignalStore properties using the withProps feature. Read more about withProps here: https://ngrx.io/guide/signals/signal-store/custom-store-properties

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants