Skip to content
This repository has been archived by the owner on Jul 23, 2019. It is now read-only.

Change ngx-forge module to use index.js #93

Closed
dlabrecq opened this issue Mar 6, 2018 · 1 comment
Closed

Change ngx-forge module to use index.js #93

dlabrecq opened this issue Mar 6, 2018 · 1 comment

Comments

@dlabrecq
Copy link
Collaborator

dlabrecq commented Mar 6, 2018 •
edited
Loading

The ngx-forge module, defined in package.json, is using bundles/ngx-forge.js instead of index.js.

"main": "bundles/ngx-forge.js",
"module": "bundles/ngx-forge.js", <<<
...

There are a few issues below, related to not using index.js.

  1. The ngx-forge.js bundle includes all of its dependencies (e.g., patternfly-ng, asciidoctor, etc.). This makes the bundle very large, which causes fabric8-ui to build very slowly. A typical fabric8-ui build is roughly 2 minutes, but ngx-forge increases that to 7.5 minutes because Angular must resolve duplicates.

As a workaround, we've added the webpack uglify plugin to ngx-launcher in order for tree shaking. This helps reduce the ngx-forge bundle size, but it's not a complete fix.

  1. Fabric8-ui cannot include its own dependencies because they are bundled in ngx-forge.js. The ngx-forge package should define a version range for its dependencies, but allow fabric8-ui to use a fixed version.

  2. Since ngx-forge.js is bundling its own dependencies, fabric8-ui unit tests are generating the following warning. Both fabric8-ui and ngx-forge have their own copies of patternfly-ng, so there are multiple instances of mobx. Mobx is a dependency of angular-tree-component, which patternfly-ng uses in the tree list component consumed by the Planner UI.

PhantomJS 2.1.1 (Linux 0.0.0) WARN: '[mobx] Warning: there are multiple mobx instances active. This might lead to unexpected results. See mobxjs/mobx#1082 for details.'

  1. The AsciidocService class includes asciidoctor.js using a require statement.

let asciidoctor = require('asciidoctor.js')();

As a test, the ngx-forge module was modified to use index.js. In this scenario, the fabric8-ui build completes much faster because Angular does not need to resolve duplicates. Unfortunately, fabric8-ui unit tests and runtime could not locate asciidoctor.js due to the require statement.

ERROR in ./node_modules/asciidoctor.js/dist/asciidoctor.js
Module not found: Error: Can't resolve 'fs' in '/Users/dlabrecq/Work/alm/repos/fabric8-ui/node_modules/asciidoctor.js/dist'

I suspect this could be why the ngx-forge module is pointing to the ngx-forge.js bundle Vs index.js? If so, perhaps we should consider moving AsciidocService to the Launcher UI (i.e., considering it's not common to both fabric8-ui and the Launcher UI)?

  1. Note that other fabric8-ui dependencies such as ngx-fabric8-wit, ngx-login-client, etc. all define "module": "index.js" in package.json
@dlabrecq dlabrecq mentioned this issue Mar 6, 2018
Closed
@dlabrecq
Copy link
Collaborator Author

dlabrecq commented Mar 6, 2018 •
edited
Loading

See fabric8-ui's mobx issue here: openshiftio/openshift.io#2461

@dlabrecq dlabrecq changed the title ngx-forge module should use index.js Change ngx-forge module to use index.js Mar 6, 2018
dlabrecq added a commit to dlabrecq/ngx-launcher that referenced this issue Mar 7, 2018
@dlabrecq
chore(app-launcher): modified ngx-forge package module to use index.js
19a01fc 
Fixes: fabric8-launcher#93
Tested via npm link with fabric8-ui.

While the ngx-forge.js bundle is still available, this helps fabric8-ui build performance because Angular isn’t resolving duplicates.
This also means that fabric8-ui can install its own dependencies instead of packages being embedded in the ngx-forge.js bundle.

- Copied HTML and CSS to the correct dist/src/app directories instead of dist/app
- Copied missing images to dist/src/assets
- Modified package.json module to use index.js
- Fixed components with missing HTML and CSS paths
- Moved styleUrls after templateUrl so grunt can properly rename files from “.less” to “.css”
- Removed duplicate trigger attributes to avoid linter attr-no-duplication errors
- bumped package version number for next release
dlabrecq added a commit to dlabrecq/ngx-launcher that referenced this issue Mar 7, 2018
@dlabrecq
chore(app-launcher): modified ngx-forge package module to use index.js
0ae3102 
Fixes: fabric8-launcher#93
Tested via npm link with fabric8-ui.

While the ngx-forge.js bundle is still available, this helps fabric8-ui build performance because Angular isn’t resolving duplicates.
This allows fabric8-ui to install its own dependencies instead of packages being embedded in the ngx-forge.js bundle.
It also eliminates the ‘multiple mobx instances’ error seen with fabric8-ui unit tests, since dependencies are essentially being duplicated by the ngx-forge.js bundle.

- Copied HTML and CSS to the correct dist/src/app directories instead of dist/app
- Copied missing images to dist/src/assets
- Modified package.json module to use index.js
- Fixed components with missing HTML and CSS paths
- Moved styleUrls after templateUrl so grunt can properly rename files from “.less” to “.css”
- Removed duplicate trigger attributes to avoid linter attr-no-duplication errors
- bumped package version number for next release
@dlabrecq dlabrecq mentioned this issue Mar 7, 2018
Merged
dgutride pushed a commit that referenced this issue Mar 8, 2018
@dlabrecq @dgutride
chore(app-launcher): modified ngx-forge package module to use index.js
176f7ab 
Fixes: #93
Tested via npm link with fabric8-ui.

While the ngx-forge.js bundle is still available, this helps fabric8-ui build performance because Angular isn’t resolving duplicates.
This allows fabric8-ui to install its own dependencies instead of packages being embedded in the ngx-forge.js bundle.
It also eliminates the ‘multiple mobx instances’ error seen with fabric8-ui unit tests, since dependencies are essentially being duplicated by the ngx-forge.js bundle.

- Copied HTML and CSS to the correct dist/src/app directories instead of dist/app
- Copied missing images to dist/src/assets
- Modified package.json module to use index.js
- Fixed components with missing HTML and CSS paths
- Moved styleUrls after templateUrl so grunt can properly rename files from “.less” to “.css”
- Removed duplicate trigger attributes to avoid linter attr-no-duplication errors
- bumped package version number for next release
arunkumars08 pushed a commit to arunkumars08/ngx-forge that referenced this issue Mar 9, 2018
@dlabrecq @arunkumars08
chore(app-launcher): modified ngx-forge package module to use index.js
1ea90cc 
Fixes: fabric8-launcher#93
Tested via npm link with fabric8-ui.

While the ngx-forge.js bundle is still available, this helps fabric8-ui build performance because Angular isn’t resolving duplicates.
This allows fabric8-ui to install its own dependencies instead of packages being embedded in the ngx-forge.js bundle.
It also eliminates the ‘multiple mobx instances’ error seen with fabric8-ui unit tests, since dependencies are essentially being duplicated by the ngx-forge.js bundle.

- Copied HTML and CSS to the correct dist/src/app directories instead of dist/app
- Copied missing images to dist/src/assets
- Modified package.json module to use index.js
- Fixed components with missing HTML and CSS paths
- Moved styleUrls after templateUrl so grunt can properly rename files from “.less” to “.css”
- Removed duplicate trigger attributes to avoid linter attr-no-duplication errors
- bumped package version number for next release
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@dlabrecq