refType: force name to writable before updating it.

[kanaka]

I discovered this will trying to get ffi to work with lumo (standalone ClojureScript REPL bundled using nexe). The original lumo issue is here.

When node is run in strict mode (node --use_strict), the attribute writable property is enforced and updating "name" will fail if the writable property is set to false. This happens when ref is used during ffi module import (and refType tries to clone a StructType object).

Here is the simple reproducer:

$ npm install ffi
$ node --use_strict -e "require('ffi')"
TypeError: Cannot assign to read only property 'name' of object '[object Object]'
    at Object.refType (.../node_modules/ref/lib/ref.js:318:14)
    at Object.<anonymous> (.../node_modules/ffi/lib/type.js:22:42)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (.../node_modules/ffi/lib/cif.js:6:12)

[TooTallNate]

This seems like an ok idea. We should add a test case or two.

[kanaka]

Okay, I can work on that.

[TooTallNate]

Just wanted to say this was a nice catch! That code has been in place for a long time and I've never noticed that the name wasn't being set correctly.

[kanaka]

@TooTallNate okay, I've added a test. Looks like it trips up some older versions of node.

With respect to the test, perhaps you could suggest a better way to construct the test type ("StructType") so that it's closer to what ref-struct provides without actually importing ref-struct and doesn't trip up the older node versions. My stab at it feels a little bit messy. It does throw the same error (in newer versions of node) if you remove the props.writable = true from lib/ref.js

[kanaka]

I tweaked the test to move the type out of the test for node 0.8 (yuck), and moved the property definition into the Object.create itself to see if that makes it pass the other complaints.

[kanaka]

So in node 0.8 it's still complaining about "SyntaxError: In strict mode code, functions can only be declared at top level or immediately within another function". node 0.10 - node 5 the name property doesn't seem to be properly defined (either writable === true, or no properties at all on the name property). node 6 passes as before.

[kanaka]

I'm thinking that perhaps I should get rid of the initial sanity check/assert of the name property. I bet that will get everything to pass except for node 0.10. Should we just pass on activating --use_strict for the test runner for that case? The error is not clear about which test or line number is causing that complaint.

[TooTallNate]

The issue with the failing tests seems to be that you're not creating a named function (i.e. you need to do function StructType () {}, rather than var StructType...)

[kanaka]

That's certainly an improvement. Now only node 0.8 and 0.10 fail. I'm going to try and get a docker container with node 0.8 going so that I can test directly without relying on Travis.

[kanaka]

It looks like mocha --use_strict just refuses to cooperate under node 0.8 even if I run it in an empty directory with a stub test file (i.e. no dependencies/modules). Are you okay with me reverting the --use_strict addition for now or do you want to keep pursuing this direction to get all tests and node versions passing with --use_strict?

[TooTallNate]

We could probably just drop 0.8 from the test suite. It's so old, it's probably time to be unsupported anyways.

[kanaka]

Okay, I updated the most recent commit to drop 0.8. For 0.12 I've commented out the initial check of the read property since writable is set to true initially in node 0.12.

[kanaka]

Sigh, adding --use_strict back to the mocha run command causes node 0.10 to fail now with a complaint that the name property is undefined. Supporting multiple versions of a rapidly changing platform is such fun!

[kanaka]

@TooTallNate so I put in hacks to check for v0.10 and v0.12 into the test. It's ugly, but I think it's worth it to be able to use --use_strict. So with those changes, the only test failing now is for node v0.10 on Windows. The appending of "*" to the name is apparently not working there. The crazy thing is that the same test works on Linux.

Should I just move that check into the conditional that checks to make sure we aren't on v0.10? Given that the appending of "*" doesn't seem critical for functionality and that this is just for v0.10 on Windows I feel like that might be a reasonable compromise: less testing for v0.10 but better testing of everything else because of --use_strict. Thoughts?

[kanaka]

I moved the name check so that it no longer fires for node v0.10 at all and everything is passing. The net effect is that for node v0.10 there aren't any asserts actually firing so it's just testing that the refType runs without an exception. The upside is that we are now running all tests with --use_strict successfully (and it is running all the new asserts for node 1 and higher).

[kanaka]

@TooTallNate is the current state of the changes (with all tests passing but fewer asserts running for node 0.10/0.12) acceptable? Anything else you would like me to address?

[kanaka]

@TooTallNate that's weird, looks like what got merge was some earlier state that didn't have the most recent test fixups I did to the PR.

[TooTallNate]

I cherry-picked just the first commit. I did a tiny refactor in 55716fd9e317ad4f0cea2a30dcb08e7cf912089e that retains all versions of Node.js still passing for me.

[kanaka]

Ah, excellent. Yes, that's much better/cleaner. Thanks for following up. When you get a chance would you be able to push an updated version to npm?

[TooTallNate]

1.3.4 published to npm.

[kanaka]

Thanks! Works great for me.