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

Switch to Nodes implementation #1

Merged
merged 1 commit into from
Feb 17, 2015
Merged

Switch to Nodes implementation #1

merged 1 commit into from
Feb 17, 2015

Conversation

jscissr
Copy link
Contributor

@jscissr jscissr commented Feb 16, 2015

There is no reason to have a different implementation/code in this module than Node. Since that's what it says to be, an "Outsource of Node's internal FreeList module".

  • package.json: changed to MIT license, that's what Node has.
  • assert function was unused
  • unnecessary arguments to Array conversion: http://stackoverflow.com/a/20375156
  • There was a check for undefined constructor. But seriously, who wants a free list of plain Objects? If you really think this is important, you can add it again or increase the major version (=breaking change).
  • Added last line in new version to keep support for shorthand without .FreeList

I already updated the version, you simply have to npm publish yet.

There is no reason to have a different implementation in this module
than Node. Since that's what it says to be, an "Outsource of Node's
internal FreeList module".
- package.json: changed to MIT license, that's what Node has.
- assert function was unused
- unnecessary arguments to Array: http://stackoverflow.com/a/20375156
- There was a check for undefined constructor. But seriously, who wants
  a free list of plain Objects?
- Added last line in new version to keep shorthand without ".FreeList"
@19h
Copy link
Owner

19h commented Feb 16, 2015

Although I understand and welcome your effort, I don't see the point. This module doesn't carry the MIT license, because it's in fact the pretty-printed compilation of what we deployed in production: freelist.coffee, not the FreeList module that Node carries. That is, I'm happy you highlighted the superfluent assertion!

I'd be willing to land and maintain your proposal if you could outline actual functional differences or deficiencies in the published version.

Cheers

REL nodejs/node#569

@jscissr
Copy link
Contributor Author

jscissr commented Feb 17, 2015

I'm afraid I don't know any functional differences, so I'll close this :)

I think it's easier for you to remove the assert function yourself, instead of me creating a new pull request. You'd anyway need to checkout the code, to do npm publish.

There were just some things like the assert or that the README is missing on npm (it was added after the last npm publish), which made me made think that this repo isn't very well maintained.

Relating to nodejs/node#569: The reason I'm interested in FreeList is because I want to make a package of Nodes http, for platforms where you have TCP Socket APIs as in Chrome Apps.

@jscissr jscissr closed this Feb 17, 2015
@19h 19h reopened this Feb 17, 2015
19h added a commit that referenced this pull request Feb 17, 2015
Switch to Nodes implementation
@19h 19h merged commit cc82b3b into 19h:master Feb 17, 2015
@19h
Copy link
Owner

19h commented Feb 17, 2015

Landed at 7bbe7f0 as [email protected]. Thanks!

@jscissr
Copy link
Contributor Author

jscissr commented Feb 18, 2015

Did you now intentionally change the license to MIT?

@19h
Copy link
Owner

19h commented Feb 18, 2015

Yes :-) I'm just against carrying Joyent in everything node-ish. Cheers

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 this pull request may close these issues.

2 participants