BranchMetrics / web-branch-deep-linking-attribution

The Branch Web SDK for deep linking and attribution. Once initialized, the Branch Web SDK allows you to create and share links with a banner (web only), over SMS, or your own methods by generating deep links. It also offers event tracking, access to referrals, and management of credits.
https://help.branch.io/developers-hub/docs/web-sdk-overview
MIT License
288 stars 101 forks source link

Make useable in all js environments #334

Open morenoh149 opened 8 years ago

morenoh149 commented 8 years ago

The branch sdk should check if window exists before utilizing it. See https://github.com/BranchMetrics/smart-app-banner-branch-deep-linking-web-library/blob/master/dist/build.js#L1845

This was only discovered while running branch in a worker thread, as window is not available in that environment.

screen shot 2016-06-15 at 2 31 33 pm
jsaleigh commented 8 years ago

@morenoh149 oh that's interesting, I don't think we've seen that one before. Thanks for the find!

6graNik commented 8 years ago

yep it is because of server-side rendering, you have lines like return navigator.isCookieEnable which will fail

Workaround may be using branch only on the client, use async chunk for example

bradyhigginbotham commented 7 years ago

Just ran into this as well... 😢

sung-hwang-zocdoc commented 7 years ago

Just ran into this as well...

ghost commented 7 years ago

IS this still an issue? trying to implement in React project and getting the same error only on "import" of the package... Anyone have a workaround?

pho3nixf1re commented 7 years ago

This is completely blocking for server side rendered apps. What is the recommended workaround? Could the global be mocked just so the import works on the server?

pho3nixf1re commented 7 years ago

As a workaround I'm lazy loading the branch library using import(). Another option may be to mock the global window and navigator server side but that could get messy.

vcostin commented 7 years ago

I've handle it with isomorphic-ensure with webpack

grydstedt commented 6 years ago

Any plan on fixing this? this is a blocker

rubinsingh commented 6 years ago

Hey Everyone,

Our Product and Technical teams have decided to not move forward with this feature request. Reason being that the WebSDK depends on information from the browser for most of our SDK features. We lose that information if it runs server-side instead.

We apologize for any inconveniences caused as a result of this decision and we look forward to servicing your needs through the browser environment.

grydstedt commented 6 years ago

@rubinsingh This has nothing to do with running branch code on the server, the problem is that you can't even import the branch sdk module on the server side which makes your developers jump through hoops to use your module. I'll even save you guys the work, here is the code:

On global module level
if (typeof(window) !== 'undefined') {
   // Do stuff
}
On init()
if (typeof(window) === 'undefined') {
  throw new Error('window not found. You might have called init() on the server');
}
morenoh149 commented 6 years ago

Indeed, @rubinsingh if you read the first post, if a developer wants to use branch in a worker thread (that's still in the browser) they have to go through hoops. It's just bad javascript hygiene to assume the code is being run with the window object available.

pho3nixf1re commented 6 years ago

@rubinsingh I concur. Like the others, our use case is not to use the SDK from within Node on the server side but to allow the inclusion of the module there for server side rendering a React app. The work around for this hurts the user experience and is very much not ideal for both the user and the developer.

morenoh149 commented 6 years ago

I don't think it's trivial to make the sdk not depend on the window object. @grydstedt I just peeked through https://github.com/BranchMetrics/web-branch-deep-linking/search?p=2&q=window&type=&utf8=%E2%9C%93 The change is going to require making a module for this project (standard practice) and update all refs to window to interact with that instead. There's also some funky stuff with activeX objects.

window.sdk_version = 'web' + config.version;

it's very presumptuous to use sdk_version for yourselves. As if the branch sdk were the only possible sdk a site might use.

@rubinsingh It's fine if you all can't address this in a certain amount of time. Simply recognize it as tech debt and add it to your Backlog. Add a help wanted tag and someone may come along and fix it for you ;)

rubinsingh commented 6 years ago

Hey Team,

Thank you for your feedback. To get our WebSDK operable in environments where the window object is not available is a significant undertaking for the team (as you've seen through searching for references to the 'window' object).

We've found another area that that would need to be re-tested/potentially re-written as a result of this change. That area is related to storage i.e. how we use application, local and cookie storage in the WebSDK to store and retrieve items related to our core deep linking functionality.

We do understand your frustration though so we've added this as a feature request in our internal WebSDK backlog.

Incase anyone in this thread finds it useful, we have an HTTP API as an alternative:

https://github.com/BranchMetrics/branch-deep-linking-public-api

@pho3nixf1re regarding using Branch with a server side rendered react app, I wanted to share a technique I used to make Branch work in this environment.

Using the example at the following location:

https://github.com/mhart/react-server-example

I made the following changes:

  1. Added script({src: 'https://cdn.branch.io/build.min.js'}) over at: https://github.com/mhart/react-server-example/blob/master/server.js#L61
  2. Then to use it in a component, I added the following lines in componentDidMount(): branch.init('insert branch key', function(err, data) { // callback to handle err or data });

at: https://github.com/mhart/react-server-example/blob/master/App.js#L18

This worked well for me. Please feel free to shoot an email to integrations@branch.io if you're having trouble with React and Branch.

@morenoh149 thanks for calling out the .sdk_version issue - we've added it to our backlog.

ljharb commented 5 years ago

@rubinsingh it doesn’t matter if it operates; it still should be requireable in a non-browser environment.

This isn’t a feature request, it’s a bug. Everything should be importable/requireable in every environment, whether it’s going to work or not.

In airbnb’s case, all react components are server-rendered, and browser-only things are invoked only in componentDidMount or event handlers.

csalmi-branch commented 5 years ago

Thanks everyone for the thoughtful discussion on this issue! I will talk with our Product team to figure how best to support this use case. I completely understand that in its current state, our web SDK might not be ideal for all web applications, especially ones that might be rendering part of the page server-side or a hybrid app of sorts, due to its dependency on the window object.

As Rubin mentioned above, the changes required to remove the dependency on the window object will not be quick. We will need to decide on the best approach to this work, to make sure we keep all functionality and features available for customers that are already using this SDK regularly, and many of those currently rely on the existence of the windows object and will not work without it. This issue has been backlogged and I will update this thread as soon as I have more details to share.

In the meantime if you have any further questions or concerns, please let me know. Or you can submit a ticket with our support team: https://support.branch.io/support/tickets/new/.

ljharb commented 5 years ago

It’ll take a few ternary statements and if blocks to make it work identically in a browser, but fail gracefully elsewhere. I wouldn’t expect it would take more than a day’s work, and that’s being extremely conservative.

reintroducing commented 5 years ago

When I ported my team's code to an SSR environment, I ran up against this. Since we don't care about Branch on the server, I was able to get around this by doing the following. We have a BranchUtils module that wraps the branch-sdk and we import that sdk like so (after our other import statements):

const branch = (EnvironmentUtils.isBrowser())
    ? require('branch-sdk')
    : null;

The usage of the methods from the branch-sdk is then protected like so (code shortened for brevity):

const BranchUtils = {
    init(key, cb) {
        if (!branch) { return; }

        branch.init(key, cb);
    },

    link(data, callback) {
        if (!branch) { return; }

        branch.link(data, callback);
    },

    setViewData(data) {
        if (!branch) { return; }

        branch.setBranchViewData(data);
    }
};

Hopefully that helps some people out until the Branch team can fix this internally.