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

Make useable in all js environments #334

Open
morenoh149 opened this issue Jun 15, 2016 · 19 comments
Open

Make useable in all js environments #334

morenoh149 opened this issue Jun 15, 2016 · 19 comments

Comments

@morenoh149
Copy link

morenoh149 commented Jun 15, 2016

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
Copy link
Contributor

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

@jsaleigh jsaleigh self-assigned this Jun 16, 2016
@6graNik
Copy link

6graNik commented Jun 30, 2016

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
Copy link

Just ran into this as well... 😢

@morenoh149 morenoh149 changed the title Undefined window Make useable in all js environments Apr 3, 2017
@sung-hwang-zocdoc
Copy link

Just ran into this as well...

@ghost
Copy link

ghost commented Jul 26, 2017

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
Copy link

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
Copy link

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
Copy link

vcostin commented Oct 4, 2017

I've handle it with isomorphic-ensure with webpack

@grydstedt
Copy link

Any plan on fixing this? this is a blocker

@rubinsingh
Copy link
Contributor

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
Copy link

grydstedt commented Jan 6, 2018

@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
Copy link
Author

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
Copy link

@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
Copy link
Author

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
Copy link
Contributor

rubinsingh commented Jan 16, 2018

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 [email protected] 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
Copy link

ljharb commented May 21, 2019

@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
Copy link

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/.

@csalmi-branch csalmi-branch reopened this May 23, 2019
@ljharb
Copy link

ljharb commented May 23, 2019

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
Copy link

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.

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

No branches or pull requests