liabru / matter-attractors

an attractors plugin for matter.js
MIT License
143 stars 31 forks source link

Fixed NPM Package #2

Closed voydz closed 7 years ago

voydz commented 7 years ago

This will fix the NPM package. Relates to #1

This is fixing the webpack export config according to the documentation. (see https://webpack.js.org/configuration/externals/)

To build I ran:

  1. yarn run build
  2. yarn run lint
  3. yarn test
voydz commented 7 years ago

Build seems to fail because of webpack v2.3.0 being installed through npm on travis. (see https://github.com/webpack/webpack/issues/4549)

Maybe we should use yarn in .travis.yml to respect the version locks?

liabru commented 7 years ago

Can you confirm if the fix is just this part:

 -  externals: {
 -    'matter-js': 'Matter'
 -  },
 +  externals: [
 +    'matter-js'
 +  ],

Correct? But how does 'matter-js' resolve to the global Matter when in the browser in this case?

voydz commented 7 years ago

@liabru Yes, that is the fix, essentially. As far as is understood the docs ['matter-js'] is (obviously) the same as {'matter-js': 'matter-js'}. This is what we need to exclude modules which are imported/required via CommonJS, etc.

Whereas {'matter-js': 'Matter'} would be used for packages (like jQuery) which exports themself to the window in browser environments.

The issue with that was, that the matter-attractor plugin would search for a Matter package to import. This would fail, of course. If you have a look at the build output, you will see, that the requires changed from Matter to matter-js.

I hope this makes any sense to you 😄 , I tried to express it the best I could, best regards

liabru commented 7 years ago

I tried this change and it doesn't work in the browser, where as the original version does. I suspect that it doesn't try to use require before it looks for the global like I hoped it would. I'm not sure yet how to resolve this.

voydz commented 7 years ago

Ok, thats odd. 🤔 In my case I installed it as an NPM package while using it with webpack (babel-loader).

// Contents of .babelrc
{
  "presets": [
    ["es2015", {
        "modules": false
    }]
  ],
  "plugins": [
    "transform-es2015-destructuring",
    "transform-object-rest-spread"
  ]
}

If I just try to import or require the current NPM package, it fails saying that I need to install the dependency "Matter".

liabru commented 7 years ago

Actually I think this is what's needed:

  externals: {
    'matter-js': {
      commonjs: 'matter-js',
      commonjs2: 'matter-js',
      amd: 'matter-js',
      root: 'Matter'
    }
  },

I'll try it out!

voydz commented 7 years ago

@liabru awesome, thank you!

liabru commented 7 years ago

Should be fixed as of 0.1.6 please try it out!