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

Doesn't work in nodejs anymore #54

Closed
heralden opened this issue Aug 21, 2019 · 5 comments · Fixed by #56
Closed

Doesn't work in nodejs anymore #54

heralden opened this issue Aug 21, 2019 · 5 comments · Fixed by #56

Comments

@heralden
Copy link
Member

After 195b9e0, imjs no longer works in nodejs. Requiring it will throw the error

/home/regen/prog/imjs/dist/im.js:14024
})(window.intermine);
   ^

ReferenceError: window is not defined
    at Object.<anonymous> (/home/regen/prog/imjs/dist/im.js:14024:4)
    at Module._compile (internal/modules/cjs/loader.js:816:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:827:10)
    at Module.load (internal/modules/cjs/loader.js:685:32)
    at Function.Module._load (internal/modules/cjs/loader.js:620:12)
    at Module.require (internal/modules/cjs/loader.js:723:19)
    at require (internal/modules/cjs/helpers.js:14:16)
    at Object.<anonymous> (/home/regen/prog/imjs-test/indexx.js:1:12)
    at Module._compile (internal/modules/cjs/loader.js:816:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:827:10)

The below change is likely the culprit.

-  "main": "build/service",
+  "main": "dist/im.js",

I guess the actual breakage lies deep in the cors fixing code, so I hope @LakshSingla has a chance to look at this?

May be related to #41 as the error also occurs at require, but I doubt it as that issue is pretty old.

I tested by doing a grunt build of the codebase checked out on that commit, and the one directly before it (where it works completely fine). Node version is v11.14.0.

@heralden
Copy link
Member Author

I looked more into it and it looks like dist/im.js is a browser-only build, so I guess the question becomes why main was changed to this, and thereby breaking the npm package.

@LakshSingla
Copy link
Member

LakshSingla commented Aug 21, 2019

Lemme take a look into this, but here's a breakdown of the changes that I made while fixing the CORS bug. This error was also popping up while I was trying to test after changing the main field, but I didn't realize that it might also come while importing the library directly into Node.

  1. The main field was changed to this because the CDN version works whereas the NPM version does not. There is no difference between them, hence NPM version should be working. On changing the main field of the NPM version, the CORS error disappeared. (By working, I mean when it is bundled in the final package)
  2. In order to fix the issue then, I tried to install jsdom-global, which allows this type of behavior in Node environment. But I was unable to make it work alongside CoffeeScript compiler. The tests were not working because of the above mentioned issue.
  3. Therefore, as a workaround, I changed the directory from where it was importing imjs for testing the library, from im.js to bundle/service.

TLDR: The CORS fix requires that the main be pointed to the final built imjs and not a component of it. Using jsdom-global might fix the problem, but I was unable to make it work then. Lemme look into it again.

@heralden
Copy link
Member Author

Thanks for the reply. Sounds like a complex and annoying problem. Best of luck!

@LakshSingla
Copy link
Member

Hey @uosl, I just created another PR that fixes this issue on my machine. Instead of changing the main field, I am adding an entry in the browser field that is picked up by the front end bundlers and changes the file accordingly. Can you please test it on your setup and let me know if it works for you as well? (Thanks to @amand1996 for this great idea)

@heralden
Copy link
Member Author

It works perfectly in my setup, and seems like the optimal solution as well. I take it you've tested and made sure it doesn't reverse your cors fix, so I merged the PR.

Thanks again for the speedy fix!

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

Successfully merging a pull request may close this issue.

2 participants