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

Remove transform-runtime #7

Merged
merged 1 commit into from
Apr 30, 2017
Merged

Conversation

neftaly
Copy link
Contributor

@neftaly neftaly commented Apr 30, 2017

Atom-store is currently broken, as transform-runtime adds a dependency which is not included. I understand why you would have it, and considered making a PR with this dep, but it's fairly large, and the actual size difference without it is trivial (even ignoring that gzip will remove the size diff entirely):

Compiled atom.js, with transform-runtime:
var _extends2 = require('babel-runtime/helpers/extends'); var _extends3 = _interopRequireDefault(_extends2);

Without transform-runtime:
var _extends = Object.assign || function (target) { for (var i = 1; i < arguments.length; i++) { var source = arguments[i]; for (var key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { target[key] = source[key]; } } } return target; };

@jameshopkins
Copy link
Owner

jameshopkins commented Apr 30, 2017

Atom-store is currently broken [...]

What's the error you're seeing and moreover, what's the dependency that isn't being included? I've been able to successfully run the tests on master with a fresh install of all the deps.

@neftaly
Copy link
Contributor Author

neftaly commented Apr 30, 2017

The tests are only run on src, but dist is broken

Error: Cannot find module 'babel-runtime/helpers/extends'
    at Function.Module._resolveFilename (module.js:470:15)
    at Function.Module._load (module.js:418:25)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/home/neftaly/immstruct/node_modules/atom-store/dist/atom.js:7:17)
    at Module._compile (module.js:571:32)
    ...

@neftaly
Copy link
Contributor Author

neftaly commented Apr 30, 2017

It probably only manifests when installing in things without a babel compile step, which is why it wasn't noticed earlier.

npm run compile
cat dist/atom.js

var _extends2 = require('babel-runtime/helpers/extends'); - not in dependencies in package.json

@jameshopkins
Copy link
Owner

The tests are only run on src, but dist is broken

In addition to the steps I described, I also changed the test targets from the src dir to dist in both test/atom.js and test/plugins.js files, and all the tests passed.

How are you yielding that error? Through the immstruct test runtime? Do you happen to have babel installed globally?

@neftaly
Copy link
Contributor Author

neftaly commented Apr 30, 2017

Yes, through immstruct (as it uses ES5 and no babel). I don't have babel installed globally. Installing babel-runtime in immstruct fixes the error (but not the problem - if you look at dist/atom.js you'll see exactly the dependency I'm referring to).

The other solution is to npm install --save babel-runtime in atom-store.

@neftaly
Copy link
Contributor Author

neftaly commented Apr 30, 2017

The test suite won't pick it up (ever), as it's a dep thats in devDependencies and not dependencies, and not an implementation error 😕

@neftaly neftaly mentioned this pull request Apr 30, 2017
@jameshopkins
Copy link
Owner

jameshopkins commented Apr 30, 2017

That's unfortunate :( Happy with removing this dependency, at least for the time being.

Will cut a new patch release in the next few minutes.

@neftaly
Copy link
Contributor Author

neftaly commented Apr 30, 2017

#8 might be better, as the issue will also exist in immutable-cursor (ack)

@jameshopkins
Copy link
Owner

We should be OK just to remove babel-plugin-transform-runtime from immutable-cursor too.

@neftaly
Copy link
Contributor Author

neftaly commented Apr 30, 2017

Ok, sweet, I'll let you do that. Thanks!

@jameshopkins jameshopkins merged commit fd4bd15 into jameshopkins:master Apr 30, 2017
@neftaly
Copy link
Contributor Author

neftaly commented Apr 30, 2017

Cheers :)

@neftaly neftaly deleted the transform branch April 30, 2017 23:42
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