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

[RFR] Better integration with module bundlers #959

Merged
merged 16 commits into from
Sep 7, 2016
Merged

Conversation

jpetitcolas
Copy link
Contributor

@jpetitcolas jpetitcolas commented Mar 4, 2016

  • Update README and UPGRADE files
  • Upgrade Getting started snippets
  • Update package.json to point on correct files

## Bootstraping your Admin

Just add a `<div ui-view>` into your HTML, add a `ng-app` attribute on your `body`, and configure the admin:
ithu
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems I missed an ALT + TAB. To delete.

@jpetitcolas
Copy link
Contributor Author

With the help of @RobinBressan, switching to RFR.

@djhi, meanwhile, can you try using this branch on your project? I heard you got some troubles building an admin app in production with master version of ng-admin. It should fix your issue.

@jpetitcolas jpetitcolas changed the title [WIP] Better integration with module bundlers [RFR] Better integration with module bundlers May 18, 2016
@Phocea
Copy link
Contributor

Phocea commented May 30, 2016

Hello,

I am actually in the process of using the ng-admin-only way because I needed to add ngCookies to my app.
As you stated, the production page is out of date and it was a painful process to get it right again. I have now something working but I am not 100% that ll the dependencies I added are needed!

Anyway, does this branch means that it will be solved by simply adding a require(ng-admin) in the js loaded by our webpack build, as well as to ngCookies. And will simply be able to declare the module when initializing the module ?

var myApp = angular.module('myApp', [ 'ng-admin','ngCookies' ]);

If so, I will put my refactor on the backboiler for now, and wont submit my changes for the production documentation :)

Thanks

@jpetitcolas
Copy link
Contributor Author

@Phocea: unfortunately for your changes, this is indeed the aim of this PR. :)


``` js
// SASS version
require('ng-admin/sass/ng-admin.scss');
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ng-admin/sass/ng-admin.scss/ng-admin/src/sass/ng-admin.scss

@jpetitcolas
Copy link
Contributor Author

jpetitcolas commented May 30, 2016

To sum up a private discussion with @djhi:

  • Move admin-config as a prod dependency
  • Update Webpack configuration sample to exclude node_modules

@Phocea
Copy link
Contributor

Phocea commented Jun 6, 2016

@jpetitcolas great :) Well this is great news really because the old way was really painful and as you stated, the production.md page was impossible to maintain.
Can't wait to have this available on main. Thanks for the rework

@Phocea
Copy link
Contributor

Phocea commented Jul 12, 2016

Hello @jpetitcolas , any update on when this is going to be merged on the main branch? No rush, but it would sort out the mess I had to implement :)
Thanks

@jpetitcolas
Copy link
Contributor Author

@Phocea you're right, this PR definitively needs some love. I take back a look on it. :)

@Phocea
Copy link
Contributor

Phocea commented Jul 13, 2016

Merci Monsieur !

```

Using a module bundler, you would also be able to generate the source map for all your JavaScript
and stylesheets, helping you to hunt even the most pernicious bugs.
Copy link
Member

Choose a reason for hiding this comment

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

s/pernicious/obscure/


### Without a Module Bundler

If you don't have a module bundler, don't worry: you can still embed `ng-admin` the old way:
Copy link
Member

Choose a reason for hiding this comment

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

s/embed ng-admin the old way/include ng-admin with a <script> tag/

@@ -9,6 +9,8 @@ Let's use the REST API offered by [JSONPlaceholder](http://jsonplaceholder.typic

Copy link
Member

Choose a reason for hiding this comment

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

I think you should revert the changes on this one, to let newcomers begin with the simplest version (<script> tag)


## Removing `ng-admin-only` build

We used to build two versions of `ng-admin`: a standalone one and a `only` one. The latter includes only files required to ng-admin. But it was a little buggy and required a lot of dependencies. Including a functional version of this only version was so painful we decided to abandon it in profit of a better bundler integration (see previous paragraph).
Copy link
Member

Choose a reason for hiding this comment

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

s/this only version/this '-only' version/ s/painful we/painful that we/

@jpetitcolas
Copy link
Contributor Author

Code review applied.

@fzaninotto
Copy link
Member

\o/

@jpetitcolas
Copy link
Contributor Author

image

@jpetitcolas jpetitcolas deleted the webpack_v2 branch September 7, 2016 13:14
@Phocea
Copy link
Contributor

Phocea commented Sep 7, 2016

+1 .... here we go, more work for me to come ;)
As this method been applied to the demo by any chance ?

@jpetitcolas
Copy link
Contributor Author

@Phocea: good point: we forget that. Need to work on it.

However, if you speak of Poster Galore, here is the related PR: marmelab/ng-admin-demo#26

@Phocea
Copy link
Contributor

Phocea commented Sep 7, 2016

@jpetitcolas Yeap meant the poster galore demo . Sorry more work for you too then :)

@Phocea
Copy link
Contributor

Phocea commented Sep 14, 2016

Hello @jpetitcolas,

I have started to use your refactor in our own webpack build and went through a bit of trouble :)
I pinpointed it to a path problem, but kept going in circle about it and could not see a solution without making changes to ng-admin structure itself.
Let me try to explain:

1.

var myApp = angular.module('myApp', [
 require('ng-admin'),
]);

The only way for webpack to find the ng-admin module is to either give a path relative to the javascript this is required from, or add an alias in webpack.config like so (for example):

  resolve : {
    root : [ path.join(__dirname, 'node_modules') ],
    alias : { 'ng-admin' : 'ng-admin/src/javascripts/ng-admin.js',}
  },

Might be something to add to the documentation ?

2.
With above fixed, I am now getting some errors when webpack is resolving the require statements present in ng-admin vendor.js. All which refer to a package directly inside the node_modules is ok, all the other are failing to find the related file.
Would it not be better to use aliases in the ng-admin webpack config to avoid that path configuration trouble.
Also the vendor.js looks like it could be simplified:

  • No need for path for angular.
  • textAngular-sanitize mentionned twice

In the end I have managed to have a it wokring with the following resolve section in my webpack config:

  resolve : {
    root : [ path.join(__dirname, 'node_modules') ],
    alias : { 'ng-admin' : 'ng-admin/src/javascripts/ng-admin.js',
              'rangy-core' : 'rangy/lib/rangy-core',
              'rangy-selectionsaverestore' : 'rangy/lib/rangy-selectionsaverestore',
              'textAngular-sanitize' : 'textangular/dist/textAngular-sanitize',
              'angular-numeraljs' : 'angular-numeraljs/dist/angular-numeraljs',
              'ui-bootstrap-tpls' : 'angular-ui-bootstrap/dist/ui-bootstrap-tpls',
              'ng-file-upload' : 'ng-file-upload/dist/ng-file-upload',
            }
  },

and ng-admin vendor.js modified to look like:

global.rangy = require('rangy-core');
global.rangy = require('rangy-selectionsaverestore');
global.numeral = require('numeral');

require('angular');
require('angular-ui-router');
require('angular-ui-codemirror');
require('textAngular-sanitize'),
require('textangular');
require('nginflection');
require('ui-select');
require('angular-translate');

require('angular-numeraljs');
require('ui-bootstrap-tpls');
require('ng-file-upload');

global._ = require('underscore');

Does this sound acceptable ?

@jpetitcolas
Copy link
Contributor Author

@Phocea: there was some mistakes in this build (we didn't see them because of the use of npm link). Can you try with the updated branch on #1189?

I'm on Gitter if you need some support. But I'm working on a brand new project using the branch related to my PR, and it seems to work.

@Phocea
Copy link
Contributor

Phocea commented Sep 14, 2016

lol .. was just about to say that I have a bit of css problem now !
Will try to grab your branch.. ! am building on windows using Maven (!!) so may encounteer some unexpected stuff.
I really have to stop making the same fixes you do ;)

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.

5 participants