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

Explicit dependency on jquery is not necessary, should be removed IMHO #52

Open
catull opened this issue Mar 12, 2018 · 16 comments
Open

Comments

@catull
Copy link
Contributor

catull commented Mar 12, 2018

In 2.0.0-beta.9 jquery was removed as a direct dependency, it is an indirect dependency.

In 2.0.0-beta.10 jquery was re-introduced.

Fact is, you don't need to specify this dependency.

Because if you leave it out, admin-lte has a dependency on jqeury and will pick it up.
You end up having node_modules/jquery anyway.

For the same reason, we should NOT explicitly include bootstrap.

Why not ?

Because jquery and bootstrap are transitive dependencies of our dependencies, most likely admin-lte.

admin-lte@2.3.11 depends on a specific bootstrap and jquery version.
admin-lte@2.4.x depends on slightly different bootstrap and jquery version.

All I am saying is, let admin-lte@x.y figure out which sub-dependencies it has.
If we specify a certain version, we are only asking for more maintenance on OUR side.

Where is the benefit of that ?

My web app has never specified jquery EXPLICITLY, because admin-lte and/or bootstrap has always had that responsibility.

Guess what, bootstrap 4 apparently has no more direct dependency on jquery.

It has a peerDependency relationship though, see: bootstrap dependency on jquery

@TwanoO67
Copy link
Owner

beta.9 was not installing jquery at all

Resulting in this error during the "yarn prod" process:
An error occured during the build: Error: ENOENT: no such file or directory, open '/Users/TwanoO/Documents/PERSO/GIT/bootstraping-ngx-admin-lte/node_modules/jquery/dist/jquery.js'

That's why it's re-introduced in beta.10

@TwanoO67
Copy link
Owner

TwanoO67 commented Mar 12, 2018

That's because we still are on admin-lte 2.3
resulting in that dependencies (where no bootstrap nor jquery are):
https://github.com/almasaeed2010/AdminLTE/blob/v2.3.11/package.json

As soon as we fixed the issue #25, we could only depend on admin-lte 2.4 (and its own subdependencies for jquery and bootstrap)

@catull
Copy link
Contributor Author

catull commented Mar 12, 2018

I see, so the goal is the same, just not achievable with admin-lte 2.3.x.

@fabioformosa
Copy link

fabioformosa commented Mar 15, 2018

About transitive dependency matter, can I ask you a question?

I'm trying to build a coreProject that imports ngx-admin-lte and add other some services and components of mine that I should use in all my projects. I build coreProject and export it.

In all my projects that include coreProject (with embedded ngx-admin-lte) I would like:

  • to customize header and footer with logoService and footerService
  • to define menu with userMenu
  • to define routes using LayoutAuthComponent
  • to inherit all utilities of coreProject that interact with theme on init.

But... I got error building the projects, because they have not access to components and services of ngx-admin-lte. Is there a way to have ngx-admin-lte passing through my coreProject ?

@catull
Copy link
Contributor Author

catull commented Mar 15, 2018

@fabioformosa Excellent points you bring up.

Just the last two days I had to put in a hack, just to override the HTML template of the user-box.

I wished there was a way to specify either a different Component or at least just provide a different layout, similar to your first bullet point.

@catull
Copy link
Contributor Author

catull commented Mar 15, 2018

@fabioformosa

On the menu and routes, I was able to define my own menu, along with routes to my own components.

Can you be more specific about the errors, I am afraid I cannot follow your concern.

Show us the error, please.

Does project A which inherits from corePropject not see service "Admin LTE User service" for instance ?

Is that a case ?

Because there is a solution for that, it is called re-exporting.

@catull
Copy link
Contributor Author

catull commented Mar 15, 2018

@fabioformosa

Ideally, you should not have to export components from ngx-admin-lte out of your coreProject in the first place.

Rather, have services from coreProject offer your own components and services only; your own components and services conceal the "inner" components and services.

@fabioformosa
Copy link

fabioformosa commented Mar 15, 2018

Hi @catull ,
I'd start by saying that I'm rather a newbie in angular, I've just begun to convert a coreProject from angularJS to angular.

Answering to your request of further details, I have:
projectA -> coreProj -> ngx-admin-lte

I would like to carry-out this scenario:

coreProj structure:
 |
 |- coreThemeModule -> ngx-admin-lte
 |- coreUtilityModule -> services, components (they could interact with theme)

ProjectA should use theme offered by coreThemeModule "as-is", except for menu and header/footer customization.
CoreProject should:

  • place a router-outlet in "main content box" of ngx-admin-lte main layout, to allow ProjectA to define routes only for inner content
  • manage all common tasks: user profile section, user box for logged user, login/signup redirect, ecc.

ProjectA shouldn't have ngx-admin-lte in package.json, neither admin-lte, bootstrap. I think that should be transitive dependencies.
However ProjectA needs to access to menuService, footerService, logoService offered by ngx-admin-lte, only importing (passing-through) CoreProject.

At the moment the error I got compiling ProjectA is:
CoreProject has no exported member 'FooterService'.

Should coreProject export ngx-admin-lte modules? Should CoreProject wrap ngx-admin-lte modules?

(I'm interested at your hack to customize user profile box, it will be my next task as well. Maybe it's worth to open an other issue to address this one)

@catull
Copy link
Contributor Author

catull commented Mar 15, 2018

ProjectA indeed won't have to know about ngx-admin-lte and all its dependencies.

My setup is a bit simpler, I have myApp -> ngx-admin-lte.
For that to work, I have some "hacks" in myApp's .angular-cli.json:

        "styles": [
            "styles.css",
            "assets/integrator-theme.css",
            "../node_modules/bootstrap/dist/css/bootstrap.css",
            "../node_modules/ionicons/dist/css/ionicons.css",
            "../node_modules/admin-lte/dist/css/AdminLTE.css",
            "../node_modules/admin-lte/dist/css/skins/_all-skins.css",
            "../node_modules/@swimlane/ngx-datatable/release/index.css",
            "../node_modules/@swimlane/ngx-datatable/release/themes/material.css",
            "../node_modules/@swimlane/ngx-datatable/release/assets/icons.css",
            "../node_modules/font-awesome/css/font-awesome.css"
        ],
        "scripts": [
            "../node_modules/jquery/dist/jquery.js",
            "../node_modules/admin-lte/plugins/jQueryUI/jquery-ui.js",
            "../node_modules/bootstrap/dist/js/bootstrap.js",
            "../node_modules/ace-builds/src/ace.js",
            "../node_modules/ace-builds/src/ext-language_tools.js",
            "../node_modules/ace-builds/src/ext-split.js",
            "../node_modules/ace-builds/src/mode-java.js",
            "../node_modules/ace-builds/src/mode-json.js",
            "../node_modules/ace-builds/src/mode-xml.js",
            "../node_modules/ace-builds/src/theme-idle_fingers.js",
            "../node_modules/ace-builds/src/theme-github.js",
            "../node_modules/ace-builds/src/theme-xcode.js",
            "../node_modules/admin-lte/dist/js/app.js",
            "assets/js/scripts.js"
        ],

@catull
Copy link
Contributor Author

catull commented Mar 15, 2018

What does coreProject's main module provider: section look like ?

@fabioformosa
Copy link

ok.
ProjectA -> CoreProject -> ngx-admin-lte

ProjectA needs to use LogoService of ngx-admin-lte but currently ProjectA can't use it. At the moment, only CoreProject can use LogoService.

I tried

import {
    User,
    MenuService,
    LogoService,
    FooterService
} from 'ngx-admin-lte';

@NgModule({
  imports: [
    CommonModule,
    RouterModule,
    NgxAdminLteModule
  ],
  declarations: [coreProjectComponent],
  exports: [
    coreProjectComponent,
    User,
    MenuService,
    LogoService,
    FooterService
  ]
})
export class CoreProjectModule { }

but it seems it's not the way

@catull
Copy link
Contributor Author

catull commented Mar 15, 2018

import {
    User,
    MenuService,
    LogoService,
    FooterService
} from 'ngx-admin-lte';

@NgModule({
  imports: [
    CommonModule,
    RouterModule,
    NgxAdminLteModule
  ],
  declarations: [coreProjectComponent],
  exports: [
    coreProjectComponent,
    User
  ],
  providers: [
    MenuService,
    LogoService,
    FooterService
  ]
})
export class CoreProjectModule { }

Any better ?

@catull
Copy link
Contributor Author

catull commented Mar 15, 2018

Services and guards are provided, components are exported.

@fabioformosa
Copy link

Oops a little oversight. However I don't get the target yet.
I created 2 repos:

simple-kaishi -> kaishi (coreProject) -> ngx-admin-lte

I've opened an issue to avoid OT at this issue
fabioformosa/simple-kaishi#1

Any help is appreciated.

I've not published kaishi in npm registry, so you should:

  • launch on kaishi npm packagr
  • create a local link with npm link inside simple-kaishi against your path to 'kaishi/dist'

@fabioformosa
Copy link

fabioformosa commented Mar 16, 2018

Update: Kaishi is now a core library that wraps ngx-admin-lte and it could export other core service as well.
Feedback: Projects, that use kaishi as core library, need to import ngx-admin-lte only to access to css and js (jquery, bootstrap). The idea, at the basis of Kaishi, was instead to hide theme implementation.

Getting back to the subject of this issue, as far as you know, is there a way thorugh ngx-admin-lte can include resources (styles, and others) in its bundle?

@TwanoO67
Copy link
Owner

As we're now providing APF format, we could maybe do something with the main "style.css", and probably manage to import that.

In the same way, we can probably import jquery,bootstrap js etc... in a main js file, for this import to be hidden.

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

No branches or pull requests

3 participants