jasmine-ajax icon indicating copy to clipboard operation
jasmine-ajax copied to clipboard

Check for a global jasmine and use it if found.

Open kring opened this issue 9 years ago • 12 comments

This makes jasmine-ajax work with Browserify. Browserify is a CommonJS environment, but it runs in a browser and doesn't play nicely with jasmine-core (or at least I had trouble using it in my project). The way we use Jasmine - and I suspect this isn't uncommon - is we include Jasmine and our Browserify-built bundle using regular script tags. So jasmine is global and requiring-in jasmine-core is incorrect, even though we are in a CommonJS environment.

This change only checks whether a global jasmine is defined. A slightly more defensive approach - but perhaps overkill? - would be to also check for particular properties on that jasmine object. i.e.: make sure it's the right jasmine.

kring avatar Mar 14 '16 00:03 kring

(I originally opened this PR editing the lib/mock-ajax.js. Oops. It's now correctly editing src/boot.js.)

kring avatar Mar 14 '16 01:03 kring

For those of us using Jasmine with Karma, this is very much breaking. fs is not available in the browser.

toddwildey avatar Mar 16 '16 23:03 toddwildey

@toddwildey you mean the original change is breaking, and this PR fixes the breaking, right? That was my experience, at least.

kring avatar Mar 16 '16 23:03 kring

No - I mean this change breaks a Jasmine + Karma setup. I fixed it by adding a "root !== window" check in src/boot.js. I would submit a PR, but for reasons I won't dive into, I'm not "allowed."

toddwildey avatar Mar 17 '16 00:03 toddwildey

Interesting, I'm using Jasmine + Karma setup myself, and I needed this change for it to work. Maybe the difference is that my specs are bundled by Browserify and yours aren't?

Can you elaborate on how it breaks? In your setup, jasmine is defined, and yet you do want to require('jasmine-core')?

kring avatar Mar 17 '16 00:03 kring

jasmine-core uses 'fs', so when jasmine-ajax includes jasmine-core, my browser is hung up on how to require 'fs' when running Karma tests.

I should add that I'm using webpack for bundling. Because of this, exports is defined, which is incurring the require('jasmine-core') call.

toddwildey avatar Mar 17 '16 00:03 toddwildey

Are you sure you don't have that problem without this pull request? Because I think that this change will make it so that require is never actually executed at runtime. However, Webpack (both with and without this PR) will see the require('jasmine-core') and bundle it in for you, because it doesn't know any better.

We're actually in the process of switching from Browserify to Webpack, so I ran into this same problem the other day. My solution was to add code like this to webpack.config.js:

module: {
        loaders: [
            {
                // Don't let jasmine-ajax detect require and import jasmine-core, because we bring
                // in Jasmine via a script tag instead.
                test: require.resolve('jasmine-ajax'),
                loader: 'imports?require=>false'
            }
        ]
    }

kring avatar Mar 17 '16 00:03 kring

Just to be clear, my experience was:

  • Latest released version of jasmine-ajax (v3.2.0): works great with Webpack + Jasmine + Karma
  • Master: broken with Webpack because webpack pulls in jasmine-core
  • This PR: still broken with Webpack, for the same reason.

kring avatar Mar 17 '16 00:03 kring

@toddwildey I saw you posted a comment describing a problem and then deleted it. Were you able to solve the problem?

kring avatar Mar 23 '16 13:03 kring

I was not - not without modifying webpack.config.js. The error is still showing up, but it's no longer breaking my builds, since that code path never gets executed during tests. So while it may be annoying, it's no longer blocking.

Providing webpack.confg.js with an alias for 'jasmine-core' can fix the problem, but it's burdensome to consumers. I ultimately don't see a solution to this without removing the require('jasmine-core') call from the code.

toddwildey avatar Mar 23 '16 22:03 toddwildey

I'll leave it to the jasmine-ajax maintainers to say for sure, but my understanding is that the require('jasmine-core') can't be removed because it's necessary when using the library under node.

Did you try the webpack.config.js changes I suggested above? I think that's the right way to deal with it in webpack. It's better than adding an alias for jasmine-core, anyway, because you really want jasmine-ajax to follow the other code path and add itself to the global jasmine object.

kring avatar Mar 23 '16 22:03 kring

It's starting to look like we'll need to revisit how Jasmine-Core is packaged up for the various environments to make this all work. Currently the thing available via require('jasmine-core'); is assuming it is being loaded in a node.js context and thus has access to things like fs. There is also some work being attempted in Jasmine-Core around this: see jasmine/jasmine#970.

Unfortunately, I haven't really worked in a CommonJS/AMD type environment enough to have opinions on what Jasmine should do here.

slackersoft avatar Apr 18 '17 22:04 slackersoft

Closing since this has long since been overtaken by changes in jasmine-ajax, jasmine-core, and likely the front end development ecosystem. We haven't heard anything about CommonJS in the browser in a number of years, so my guess is that there's no longer anything Jasmine needs to do here. If there is, it's probably going to require a new approach based on the current state of the world.

sgravrock avatar Nov 05 '22 16:11 sgravrock