search

dougwilson / nodejs-depd

Deprecate all the things
MIT License
325 stars 87 forks source link

Minor changes for basic browser compat #15

Closed wesleytodd closed 9 years ago

wesleytodd commented 9 years ago

Totally understand if you don't want to take this, but this make the code atleast RUN in chrome. And it shouldn't effect node execution at all. Anyway, thoughts?

dougwilson commented 9 years ago

Is there a way we can do this without modifying the code? For example, this change seems fine, but I cannot make it without tests included here that fail without it and pass with it, otherwise I can pretty much guarantee it'll be forgotten about and some release will just break it again.

Is there something I can add to package.json? I'm not familiar with anything on the front-end, but I know there are a lot of people who use browserify and I think that lets you redefine source files for browserifying modules, right?

wesleytodd commented 9 years ago

Do the tests not pass for you? I ran the tests and they pass for me. I didnt run them in the browser tho....

There is a way to totally not run the module using browserify, but that means that we don't get the depreciation warnings in the browser code. Which is the whole point, lol. I don't know of a way to use this without getting it to actually run in the browser.

I am running this in code i am prepping for our production environment and it seems to be working fine. I can let you know if it does once we get some load on it.

dougwilson commented 9 years ago

Do the tests not pass for you? I ran the tests and they pass for me. I didnt run them in the browser tho....

The tests pass just fine. What I'm saying is that say I accept this PR and add another process.stderr.isTTY somewhere else later because I forgot about this. The tests pass, I'll publish, and you'll end up with a broken dependency. Basically, there is no test included here ensuring I don't continue to break browser support is all.

I am running this in code i am prepping for our production environment and it seems to be working fine. I can let you know if it does once we get some load on it.

Let me know. How are you getting this module into the browser, though? Are you using browserify or something else? Browserify makes it simple to refactor the written location such that it can be injected into this module and it can write to different locations based on that, which would be a better solution.

wesleytodd commented 9 years ago
  1. I misunderstood you. I am not sure how we would test that without running them in a browser environment. We talked about that on another one of your projects and I have yet to deliver an automated solution, so we can table this until I come through on that.
  2. Probably would make #1 (not intended to be a link to issue number 1) irrelevant. Are you referring to transforms or plugins? I have not written a browserify transform yet, but I can look into it. Browserify also offers the ability to swap out an entire module, but like I mentioned in my previous comment, I do want to run the depreciation warnings in the browser if I can.
wesleytodd commented 9 years ago

Soooo, sad to say, this is not the only issue I ran into. Error.captureStackTrace is actually only a V8 thing, so there is no way to get browser compatibility out of that without a re-write. So I am going to close this ticket and probably write/find a cross browser compatible library. Hopefully it will have the nice features this module has :) Thanks anyway!

dougwilson commented 9 years ago

Ah. For reference, there was issue #14 , but that was a simply fix: the ast-types can simply define a browserify rule to simply leave out depd. I can always do something here, but without a test suite running in browsers to even know if it works, I'm not sure. I'm not really a front-end kind of guy :) But yea, the stack trace stuff is pretty integral to how this module works, such that the warnings can point to the calling location :(

wesleytodd commented 9 years ago

Yeah, no worries. When I got it working in Chrome so easily I thought it might be good to go. And if I cant use it in both sides of my app then I would rather write something simple that i can use on both. And I promise now that we have launched our app I will have time to integrate some browser test for the express router.