mohsen1 / json-formatter

Angular directive for collapsible JSON in HTML
http://azimi.me/json-formatter/demo/demo.html
Other
372 stars 85 forks source link

Publish to NPM plus support for Webpack + CommonJS Package Manager #27

Closed benoror closed 9 years ago

benoror commented 9 years ago

Motivation

Hi!

I'm in the process of refactoring an app to support modern JS tooling like NPM(-only), Webpack & ES6.

Code Change

A small change is needed to support CommonJS.:

/* commonjs package manager support (eg componentjs) */
if (typeof module !== "undefined" && typeof exports !== "undefined" && module.exports === exports){
  module.exports = 'jsonFormatter';
}

I have done other PRs for some of my dependencies:

benoror commented 9 years ago

Update: I just found the package already in npmjs: https://www.npmjs.com/package/jsonformatter

I will PR the README.md accordingly

mohsen1 commented 9 years ago

I completely agree. Bower is redundant and unnecessary. I'm actually in process of getting rid of Bower in my projects.

mohsen1 commented 9 years ago

But I don't think you need CommonJS style exports for AngularJS directives since it uses angular global object to define an AngularJS module. When I refactor this for AngularJS 2 I will make sure to use ES6 export to make it play nicely with Angular2

benoror commented 9 years ago

:+1: Ok cool!

Den-dp commented 9 years ago

@mohsen1 Hi, If you don't want to have changes in json-formatter.js I can suggest you another unobtrusive solution with CommonJS-related code in index.js.

I'm asking for this because now I'm blocked to use this angular module with webpack.

benoror commented 9 years ago

@Den-dp I was able to use it in webpack with (es6 sugar):

import jsonFormatter from 'jsonformatter';
import 'jsonformatter/dist/json-formatter.css';

/**...**/

export default angular.module('myModule', [
    'jsonFormatter'
    /**...**/
  ])

My webpack.config.js

Idea taken mainly from: http://angular-tips.com/blog/2015/06/using-angular-1-dot-x-with-es6-and-webpack/

Den-dp commented 9 years ago

@benoror Thank you for the quick reply.

There is the convention that all external modules are following is to simply export the name of the module.

If you look at the end of "Config functions" paragraph of your article you will find a good example of the use of the angular module in ES6.

With that in mind after my PR the code above can be rewritten as following

import jsonFormatter from 'jsonformatter';
import 'jsonformatter/dist/json-formatter.css';

/**...**/

export default angular.module('myModule', [
    jsonFormatter //<--
    /**...**/
  ])

Notice that there are no hardcoded strings in deps array

benoror commented 9 years ago

Exactly, that's what meant in my first post :point_up: and advocate for :smile:

What I am saying is that you can temporarily use the string for the modules that don't yet support that convention

Den-dp commented 9 years ago

@benoror Yes, I understand that our solutions were more less equal… just tried to suggest another way to solve this issue.

What I am saying is that you can temporarily use the string for the modules that don't yet support that convention

I know. The only thing that unclear for me is why we can't have those changes now and why we should use workarounds with strings :)

benoror commented 9 years ago

Completely agree :wink: