keen / keen-analysis.js

A light JavaScript client for Keen
https://keen.io/docs/compute/
MIT License
40 stars 15 forks source link

Meteor loads incorrect HTTP handler, ignores object form of the "browser" field in package.json #15

Closed myktra closed 6 years ago

myktra commented 8 years ago

Keen's CORS restrictions don't seem to allow the use of this library from the browser when added in npm package form to a Meteor application.

If the script is served from the Keen CDN, as documented here – you'd need to use the kadira:dochead package to add the script dynamically – all works fine, but then you don't appear to need this package installed. There are some obvious benefits to using the npm version in terms of version control and dependency management.

Is this by design, for security reasons, or am I perhaps missing something?

To reproduce this behavior, follow these steps:

  1. Install Meteor.

    curl https://install.meteor.com/ | sh
  2. From command line, create a new Meteor project.

    meteor create myapp
    cd myapp
  3. Add the keen-analysis package and start your app.

    meteor npm install keen-analysis --save
    meteor npm start

    Note that Meteor packages the keen-analysis scripts and serves them to the browser. The Keen object is then available for any browser-based code to execute.

  4. Add the following code to client/main.js:

    // client/main.js
    Template.hello.onRendered(() => {
     const client = new Keen();
     client
       .projectId('<PROJECT_ID>')
       .post(client.url('queries', 'count'))
       .auth('<READ_KEY>')
       .send({
         event_collection: '<MY_COLLECTION>',
         timeframe: 'this_14_days'
       })
       .then(res => console.log('results!', res))
       .catch(err => console.error('error!', err));
    });
  5. Hit http://localhost:3000.

    Observe via Chrome's developer tools that the query request is rejected due to a CORS preflighting issue.

    Response to preflight request doesn't pass access control check: A wildcard '*' cannot be used in the 'Access-Control-Allow-Origin' header when the credentials flag is true. Origin 'http://localhost:3000' is therefore not allowed access. The credentials mode of an XMLHttpRequest is controlled by the withCredentials attribute.

  6. Using kadira:dochead, using the CDN version works fine:

    meteor add kadira:dochead
    // client/main.js
    // import Keen from 'keen-analysis';
    import { DocHead } from 'meteor/kadira:dochead';
    
    .
    .
    .
    Template.hello.onRendered(() => {
     const keenScript = 'https://d26b395fwzu5fz.cloudfront.net/keen-analysis-1.1.0.js';
     DocHead.loadScript(keenScript, () => {
       // do the stuff with Keen, it works...
     });
    });
dustinlarimer commented 8 years ago

Hey, @myktra - the error you are seeing is related to Meteor's incomplete support of the package.json "browser" field: https://github.com/meteor/meteor/issues/6890#issuecomment-214032062

In this case, the server-side HTTP handler is being loaded, which isn't intended for client-side use. The solution you found works since it is loading the correct client-side functionality. I'm trying to think of a better work-around that we might support in the SDK, but it looks like the Meteor might pick this issue up soon.

Thanks for reporting, and I'm really sorry about the headache!

myktra commented 8 years ago

@dustinlarimer thank you for the analysis; does implementing something like this satisfy both Node.js and Meteor users? Or do you think that's a step in the wrong direction?

https://github.com/axic/keccakjs/pull/3/commits/c95501f9e55c1b7bad1e51ef2eaef5118ad15ab5

dustinlarimer commented 7 years ago

Hey @myktra, sorry for the late reply here. The type of workaround you're suggesting would certainly work, but unfortunately this will require a hefty rewrite of the core http-handling logic. I can't say for certain if or when we can fix this, but it's definitely on my radar.

myktra commented 7 years ago

OK, thank you for the update! Workaround seems manageable for now.

adamkasprowicz commented 6 years ago

https://github.com/meteor/meteor/pull/9311