-
Notifications
You must be signed in to change notification settings - Fork 38
StoreDevtoolsModule.instrumentStore not compatible with AOT #48
Comments
@born2net the issue is in I've fixed this and it worked (AOT succeeded) with instrumentStore() but I won't have a PR until later when I am home and can run all the tests (run out of time for now). This is the commit In the meantime, if this is blocking you, clone that fork, npm install, and build the release with Then the release folder is what it goes in your If you encounter other issues, or have suggestions, post them here, as we want a good PR. I haven't contributed to this project before. |
TX SO MUCH. |
I am ready to test whenever needed... |
Great! I just got home. |
@born2net ok! Nothing like working from your own machine. Tests are passing fine. (Better to be safe). You can go ahead and test in your project with the instructions I posted earlier or I can publish you a temporary build to test and use as it might take a bit (days to weeks?) for a contributor to accept my PR and version it. |
great, so I can test now? |
Closed PR for now. Needs work. I was troubleshooting further before giving you a build, it works with ngc (in one project I use rollup and ngc, this is the same bundler store dev tools uses to create the build). After troubleshooting further, I'm suspecting the issue for this is in this line as you can see it is the main difference between instrumentStore() and the instrumentOnlyWithExtension method. This one uses useValue, I've run into that in the past and using a factory (as you can see in all the other providers), fixes it with AOT when exporting that factory outside the class. But, as I was attempting to do this, I run into the issue that this provided value depends on options when instrumenting the store This usually is not a problem:
but that requires If anyone has any ideas, or leads, that would be awesome. Off to bed now 😵 |
A potential fix (but would be a breaking change) would be for the user to pass a factory into instrumentStore instead of a plain object. import {StoreDevtoolsModule} from '@ngrx/store-devtools';
export function createInstrumentOptions() {
return {
maxAge: 5,
monitor: monitorReducer
}
}
@NgModule({
imports: [
StoreModule.provideStore(rootReducer),
// Note that you must instrument after importing StoreModule
StoreDevtoolsModule.instrumentStore(createInstrumentOptions)
]
})
export class AppModule { } |
+1 |
or maybe create a new function if you don't want a breaking change. |
could also modify the existing function to accept either a function OR the plain object since that still works with JIT. Ill fork and see how it looks |
@juanlizarazo you could use DI to pass the options into the fatory |
A workaround is to use the import { STORE_DEVTOOLS_CONFIG } from '@ngrx/store-devtools/src/config';
export function noMonitor() {
return null;
}
@NgModule({
declarations: [
AppComponent
],
imports: [
BrowserModule,
FormsModule,
HttpModule,
StoreModule.provideStore(rootReducer),
StoreDevtoolsModule.instrumentOnlyWithExtension()
],
providers: [
{
provide: STORE_DEVTOOLS_CONFIG, useValue: {
maxAge: 20,
monitor: noMonitor
}
}
],
bootstrap: [AppComponent]
})
export class AppModule { } |
if there is a solution ill close my PR |
Here is a link to the closed PR if interested in my thought. #50 |
@deebloo I think a solution is needed for those who don't want to use the plugin, but it needs to be backwards compatible with using an object/function similar to what |
yes that works, we can close the PR!!!! |
if you want me to keep the bug open let me know |
You can close it. Those who search will still be able to reference it |
ok |
I will open a bug to add to docs... |
Sweet!
@deebloo I tried, similar to what you were doing in your PR but most tests were breaking as they expect the exception to be thrown even before the provider object is returned. Of course I didn't succeed. @brandonroberts Thank you for providing us with a solution! Definitively adding a new method would be best or introducing a breaking change, as you guys suggest. I was working towards a patch to get it to work with neither api breaking changes nor new features through exposed "public" methods and aiming to keep the existing test contracts.
@born2net Even better! you can submit a PR updating the docs. |
Wondering why this issue was closed? It doesn't provide a solution when someone wants to use store-log-monitor. This only works with the chrome extension, right? Although the suggested solution works like a charm (thanks @brandonroberts!), it's probably not a solution if you want to debug things cross-browser. And I must say, I like the Angular 2 implementation of the monitor more then the chrome extension, but that might just be me :). |
I can re-open if needed, let me know guys... |
I'll keep it open until a solution for both options is supported with the latest iteration of AOT |
StoreDevtoolsModule.instrumentOnlyWithExtension()
works with AOT.StoreDevtoolsModule.instrumentStore not compatible with AOT
getting an error of:
And we rely on
instrumentStore
as we need to provide maxAge, otherwise our dev tools will not work (large data set)can we please fix it so it works just like
instrumentOnlyWithExtension
thanks as always,
Sean
The text was updated successfully, but these errors were encountered: