New typescript types do not work with TrackJS on window about trackjs-package HOT 17 CLOSED

Hey @aaronjensen! Thanks so much for the report and the example. I'm checking on this right now.

from trackjs-package.

@aaronjensen your example seems to work great, but I'd like to make it a bit more explicit that TrackJS hangs off of window, and not simply a global. If I modify your example like this:

declare global {
  var TrackJS: TrackJSStatic | undefined;
  interface Window {
    TrackJS: TrackJSStatic | undefined;
  }
}

declare var TrackJS: TrackJSStatic
export { TrackJS }

Then the definitions specify that we can either use it globally like TrackJS, or accessible off of window.TrackJS. I think this should make it easier to use, especially with your other issue #34 where we should check if window.TrackJS is truthy before referencing it.

from trackjs-package.

Works for me. By the way, I'd probably recommend export default TrackJS so it plays well with webpack externals.

from trackjs-package.

@aaronjensen we don't do a default export from the module definition either. I thought the default module problems were fixed in webpack.

from trackjs-package.

Yeah, I meant from the module as well. Oh, does externals trackjs: 'TrackJS' work with a named export? I didn't think it did.

from trackjs-package.

@aaronjensen I'm not sure I understand. Do you load the agent from our CDN and reference it as a webpack external? Can you tell me more about your setup?

from trackjs-package.

Yes, exactly. I don't actually do that right now (because I can't afaik), but that's exactly the use case that that supports. It has the nice effect of allowing someone to include the trackjs package in their package.json for the types but actually load it via the cdn in production, using webpack externals.

Don't get me wrong, I like named exports, I just think that for libraries that could also be available on window exporting as default is less surprising. Alternatively, exporting each function individually so you could import * as TrackJS from 'trackjs' is another thing that libs commonly do. It's not a big deal, it'd just fit better with the pattern that other libraries employ (like React) so it'd be less surprising.

from trackjs-package.

I've been playing with an example of this in a create-react-app setup, and it seems to work fine currently. I have the following setup:

index.html

<script src="https://cdn.trackjs.com/agent/v3/canary/t.js"></script>

webpack.config.js

module.exports = {
  externals: {
    trackjs: 'TrackJS'
  }
}

index.ts

import 'trackjs';
TrackJS.install({ token: '123' });
TrackJS.track('hey');

This seems to support your use case.

from trackjs-package.

In this case, I don't think a named or default export matters, because now we depend on the global and window types just added. There is a draft of this published to npm now, you can see it with

npm install trackjs@beta

Will this work for you?

from trackjs-package.

Yes, that works. It's a bit odd to use externals combined with an assignment-less import because it means that if I were to remove the external, this would stop working as the import is actually inert.

Personally, I'd probably prefer to see a mixpanel style async import (though with a proper async script tag rather than script injection):

https://help.mixpanel.com/hc/en-us/articles/360000865566-Set-up-Your-Tracking

It has the advantage of declaring the entire api and making it available right away but not blocking the page while loading the script itself. Once the script loads, it'd process any queued errors/identifies/etc.

That combined w/ a default export would allow me the most flexibility. I could use externals if I chose, or I could import the actual lib. If I used externals, TrackJS would always be there, even if the lib was blocked. This would mean that the types wouldn't need the | undefined on the global declarations.

from trackjs-package.

Actually, I think I should backtrack on the "Yes, that works". Unfortunately, if the script doesn't load, webpack externals throws. They really expect the external to be there, so I would need to declare the var myself if the external script fails to load.

Oh, and I realize now the suggestion about the mixpanel method is tricky/maybe a no-go because of all the places you need to hook into to capture errors. There would be a window where those weren't caught unless you actually hooked into those places as part of the snippet which could be a lot of code.

from trackjs-package.

Yeah, I agree it's a bit odd, but I think this is a bit of an edge case. Alternatively, but also odd, would be:

webpack.conf.js

module.exports = {
  externals: {
    trackjs: 'window'
  }
}

index.ts

import {TrackJS} from 'trackjs';
if (TrackJS) {
  TrackJS.install({ token: '123' });
  TrackJS.track('hey');
}

You are correct that this could still cause an issue if the script fails to load. However, you are in a position to do safety checking, like I did with the example. More than this, I think that would best be handled by including the script in your bundle directly, rather than referencing it from our CDN.

I am familiar with the Mixpanel style of loader you referenced, and the issue you pointed out with it. We have not made such a snippet available because delaying the loading of the script will cause a gap in error data during that important initialization phase.

I think the plan is to move this to our Canary release later today, then push prod sometime over the weekend. Let me know if you'd prefer other changes to the typings.

from trackjs-package.

I thought about that, but it was too terrifying to try 😄, but you're right, if that works it could solve the issue w/ the named export.

Is your recommendation to include the script in our bundle? My thinking was:

1. This project isn't updated that often and I'd probably rather have the bug fixes from the cdn version.
2. It can load earlier so it may catch more errors.
3. It can load from a separate domain and so may be more parallelizable.

The types themselves seem fine, thank you. If anything, I can import them in tsconfig to ensure that TrackJS is in scope according to typescript.

from trackjs-package.

By the way, just for my own understanding why did you decide to use a named export rather than a default? Is the way to umd in the module world changing?

from trackjs-package.

@aaronjensen we do recommend bundling the script with your app, if you have that capability. While you don't get updates to the script, we do fully support all agents, all the way back to 1.0.0. If the agent works for you when you deploy, the likelihood of bugs is low. They usually emerge when we add something new, or a new browser API is released.

Bundling and hosting he script yourself also avoid privacy-blocker extensions. Occasionally, the TrackJS domains get added to a block-domain list, which prevents the script from activating. By hosting locally, some of these issues can be avoided.

It is also likely faster, as the DNS lookup and SSL handshake for reaching out to a new domain probably take longer than downloading the actual bytes from our CDN in many cases.

We chose to go with a Named Export of a Default Export for two reasons: first of all, it seemed more common in the other packages. While we saw both styles, newer packages seems to use more naming. Secondly, we intend to add more capability to the module agent for other platforms. Having each of the platforms named will allow users to be more explicit about what they want so the unused components can be left behind when bundling. We're imagining something like this:

import { TrackJS, TrackJSServiceWorks, TrackJSReactNative, ... } from 'trackjs';

Hope that makes sense!

We're deploying the typescript definition changes now, so I'm closing this issue. Please let me know if there is anything else I can help with!

from trackjs-package.

Thanks @toddhgardner, that's helpful.

I think the main thing that threw me off on the named exports is that the cdn version doesn't use them, but it's web only.

By the way, I'm sure you're thinking about this, but with your plan to bundle multiple platforms in the trackjs package you'll want to be careful to ensure that tree shaking is possible and works. I wouldn't want to ship react native and service worker code as part of my bundle. I'd probably prefer to import from trackjs/web just to be certain and not have to rely on webpack to tree-shake.

Cheers!

from trackjs-package.

This has been deployed as 3.0.1

Thanks!!

from trackjs-package.