Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

config: null uri props in nerf-dart #8163

Closed
wants to merge 1 commit into from

Conversation

silverwind
Copy link

Deleting a uri's properties will not work anymore in io.js 2.0.0 as the properties are now getters/setters and deleting them will delete the getters/setters themselves from the instance. Nulling the properties has the same effect, while also bringing a performance gain over delete.

Related: nodejs/node#1591

Deleting uri properties will not work anymore in io.js 2.0.0 as the
properties are now getters/setters and deleting them will delete the
getters/setters themselves from the instance. Nulling the properties
has the same effect, while also bringing a performance gain over delete.

Related: nodejs/node#1591
@othiym23
Copy link
Contributor

othiym23 commented May 3, 2015

Thanks for putting this together, @silverwind. This change appears to cause some breakage elsewhere in npm (perhaps npm-registry-client?), because I'm getting some failures in the npm-registry-couchapp tests, which just goes to reinforce my belief that this is not something that we should just hotfix in the version of npm shipped with io.js.

I'll keep digging and see if I can figure out what's going on.

@silverwind
Copy link
Author

Interesting. That means setting null isn't equivalent to delete. I wonder where it differs, as I assumed it would behave exactly the same in the old url implementation.

@dougwilson
Copy link
Contributor

0.8 does behave the same; looking at the Travis CI, the failure is from a timeout in the whoami test.

@othiym23
Copy link
Contributor

othiym23 commented May 4, 2015

@silverwind I think the problem may actually be on my laptop, because every since I first pulled that branch, I haven't been able to get the tests to pass, regardless of what branch of npm I've got checked out. So I think it's safe to say this isn't your problem. I'll dig into this a little more tomorrow.

@dougwilson There's something up with tap under 0.8. I've been poking at it for about a month now, but haven't had the time to chase it to ground. There are intermittent spurious failures throughout the test suite under 0.8, so unless that particular error is repeatable for one of us, I wouldn't worry about it.

@othiym23
Copy link
Contributor

othiym23 commented May 5, 2015

Sorry to reverse course here, but after lengthy discussion with @isaacs about not just this change but the larger changes to url represented by nodejs/node#933 / nodejs/node#1561, we've decided these changes don't make sense:

  • if Node is breaking npm, that's a serious problem for io.js, rather than the other way around. If npm has problems with these changes, other applications are definitely going to have similar problems.
  • even if io.js makes breaking changes to url, there needs to be a much better deprecation process. I'm agnostic about whether it's a feature flag or putting the new version in userspace or just waiting another major release with some deprecation warnings, but either way, the changes to the module are much more significant than they initially appeared, and need to be handled systematically
  • for what we're doing with it, delete is the right tool for the job.

Thanks for putting this together, though! I appreciate the quick turnaround on sending us a patch along with filing the issue!

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

Successfully merging this pull request may close these issues.

3 participants