-
Notifications
You must be signed in to change notification settings - Fork 240
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
[BUG] Does not work with Closure Compiler #1276
Comments
We don't internally have anything scheduled to make App insights lib Closure Compiler friendly, but that doesn't mean that someone from the community can't provide some insights and PR's to make it so. Another option that you would have would be to not consume the NPM's and instead use the CDN version (raw JS), if you take this path I would suggest that you have some form of small wrapper that manages the loading / initialization as your own module (basically similar to the snippet), you could also use the snippet but this will populate a global variable with the instance reference. |
@MSNev Thanks for the reply. The problem I have with the snippet is that our app is a widget that is injected into other sites. Since it puts it in the window object, if the other (parent) site would use application insights in the same way, the 2 objects would collide. Is there a way to make the snippet load it into something else, e.g. window.myscope.applicationinsights, to get around that problem? Also, do you have some example somewhere, of this wrapper that would replace the snippet? |
For the new snippet, I exposed the internal name setting so you can do this. The other approach would be this on (https://github.com/Microsoft/ApplicationInsights-JS#alternative-setup-method-include-ai-js-sdk-script-and-initialize-statically) and in this case you could put the instance anywhere you like (including in a closure so its not public). |
@MSNev Thanks for the reply. I've seen that name setting, I'll try it that way. The depth would be a nicehave, but it's not a show stopper. We can come up with a name that won't collide with someone else. From the docs I was worried about this part:
I was under the impression that this I'll check out the other approach you mentioned, as well |
You are correct and this is also why I added this specific note to call out this possible race condition when more than 1 snippet instance is being used and both attempt to update the appInsightsSDK, this was also a pre-existing issue with the earlier versions of the snippet and it's not an easy issue to completely unrole... |
So I guess that rules out the snippet, since even with different names, the 2 instances can still collide. Your last approach seems like the only way to go, I'll give that a try. |
@MSNev One more question. I'm trying to go with the last approach you suggested. Is there a types package for this lib, so I don't have to define the same interfaces, or use "any" types all over the place? I've found this one, but it seems like it's for an older version https://www.npmjs.com/package/@types/applicationinsights-js |
Types are built into this library and should be picked up by your IDE & Typescript automatically. If you need to manually point to them, they are located here:
|
I know that they are built in, but when I load them, closure compiler scans the whole lib and crashes, since it's not compatible. Regardless of closure compiler, it would be much cleaner to have types in DefinitelyTyped, so the types and the implementation do not mix. If I just want to use the snippet, I shouldn't be forced to install the whole lib, just to get the types |
If the types where defined in a single rollup .d.ts (rather than the current individual *.d.ts files) would that assist / resolve the issue? |
Maybe, not sure, but I don't think so. You still need to install the whole lib, and include stuff from it. |
Adding build-time generation of a single *.d.ts and *.rollup.d.ts to the build to see if that would assist. These would also form the basis for any form of Definitely Typed definition, but as we generate these as part of the build and they will be included in npm it should not be required to upload and managed versions there as well -- at least that's the initial goal. |
…tion as part of the build. - Possible fix for #1276
…tion as part of the build. - Possible fix for #1276
…tion as part of the build. - Possible fix for #1276
As part of the #1475 and #1467 PR's the builds and NPM packages will now contain 2 new unified *.d.ts files for the releases, these will be located in the dist/ folder of each package, so for the AISKU (applicationinsights-web) they are called
|
v2.6.0 is now fully deployed to all CDN endpoints |
This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description/Screenshot
Hi,
I have an Angular 9 project which consists of 2 projects: app and libs. We compile everything using Google Closure Compiler (advanced mode), using the pipeline from this project: https://github.com/steveblue/angular2-rollup
Now I'm trying to use your project in the libs project. With the "normal" angular compiler (ng) it works fine. But I can not get it to work with the Closure Compiler. I'm not an expert on this, but my guess is the problem is that none of your builds are processed by tsickle, and don't get any annotations generated, needed by the Closure Compiler.
I've tried to import it from all of your builds, adding the following to my closure.conf file:
The result is the same in all 3 cases, a closure error:
ERROR - [JSC_JS_MODULE_LOAD_WARNING] Failed to load module "@microsoft/applicationinsights-web"
Maybe I'm just missing something. If so, and if anyone managed to get this lib working with the Closure Compiler, could you please share how to do it?
If not, do you have any plans of adding this support for making this lib Closure Compiler friendly?
Currently, this is a big issue for us. We would like to use Application Insights, since we have everything else on Azure, but this is a show stopper. The only alternative we have so far (if that is even possible) is to ditch this lib, and use the rest API directly
The text was updated successfully, but these errors were encountered: