Skip to content
This repository has been archived by the owner on Sep 4, 2019. It is now read-only.

Fix es6 build and add Steal compatibility. #13

Merged
merged 1 commit into from
Nov 27, 2015

Conversation

marshallswain
Copy link
Member

@daffl

I've fixed the exports on src/sockets/index.js so that the Babel build does not strip socketio and primus from the final export. This line in client.js

Object.assign(client, rest, sockets);

was getting compiled to

_extends(client, _index2.default, _index4.default);

But _index4.default was undefined. Modifying the exports fixed the issue.

I've also added the events and querystring core node modules to the dependencies, so they get included with the build. This lets Steal use the main package instead of the dist/feathers.js file.

Finally, I've added /index to a couple of imports because Steal was looking for a file called ./rest.js instead of ./rest/index. The same goes for ./sockets.

Closes #12 .

@daffl
Copy link
Member

daffl commented Nov 26, 2015

Those are good fixes although you could just map feathers-client to dist/feathers-client for Steal. That should include everything (the Browserified events etc.).

I'm still not happy how Steal doesn't ship with the Node built-ins by default (I think that is what https://github.com/stealjs/node-browser-builtins is supposed to be for). Everything else in this PR is fine but it would be nice to get rid of the events and querystring shim packages.

@marshallswain
Copy link
Member Author

Yeah. It doesn't seem very intuitive to have to install modules that are pre-packaged with Node. Isn't there an EngineDependencies or something like that for Steal? Could the browser be one of those engines to have it install those specific dependencies for Steal?

@daffl
Copy link
Member

daffl commented Nov 26, 2015

I would take the dependencies out here and try it with https://github.com/stealjs/node-browser-builtins or mapping to dist/. The other changes are fine but I don't think we should start adding dependencies or special configuration for every module loader (their whole point is to work transparently imho).

@marshallswain marshallswain force-pushed the fix-build-plus-steal-compatibility branch from 6d0a6de to aea04ea Compare November 26, 2015 18:05
@marshallswain
Copy link
Member Author

Ok. All good points. Take a look, now.

The dist/ build will probably work with Steal with these changes. I think it was only not working earlier because of the exports issue causing feathers.socketio to be missing. I should be able to just do

import feathers from 'feathers-client/dist/feathers';

@marshallswain
Copy link
Member Author

@daffl You mind if I merge and release this?

@daffl
Copy link
Member

daffl commented Nov 27, 2015

I had issues with the add-module-exports plugin not setting module.exports if there was another export in the module. Can you double check if it does? I actually added a test for it to the plugin generator:

it('is CommonJS compatible', () => {
  assert.equal(typeof require('../lib'), 'function');
});

And changed the npm test script to:

"test": "npm run compile && npm run jshint && npm run mocha"

@marshallswain marshallswain force-pushed the fix-build-plus-steal-compatibility branch from aea04ea to 9401add Compare November 27, 2015 19:23

it('is CommonJS compatible', () => {
console.log(require('../lib/client'));
assert.equal(typeof require('../lib/client'), 'function');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably needs to be 'object' ;)

@marshallswain marshallswain force-pushed the fix-build-plus-steal-compatibility branch from 9401add to 10663c2 Compare November 27, 2015 19:40
marshallswain added a commit that referenced this pull request Nov 27, 2015
@marshallswain marshallswain merged commit 681918c into master Nov 27, 2015
@marshallswain marshallswain deleted the fix-build-plus-steal-compatibility branch November 27, 2015 19:47
@daffl daffl mentioned this pull request Nov 29, 2015
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.

npm package is broken.
2 participants