Skip to content
This repository has been archived by the owner on Sep 24, 2020. It is now read-only.

NPM module configuration #31

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

davidmeirlevy
Copy link
Contributor

No description provided.

Copy link
Owner

@ducksoupdev ducksoupdev left a comment

Choose a reason for hiding this comment

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

Thanks again for the work you have done, this is an excellent feature and I'm excited about integrating it.

Just a few things - apologies :)

@@ -15,7 +15,7 @@
<collapse id="navbar" class="navbar-collapse" v-model="showNavbar">
<ul class="nav navbar-nav">
<li v-for="link in links" v-bind:class="{'active' : $route.path == link.path}">
<router-link v-bind:to="link.path">\{{link.name}}</router-link>
<router-link v-bind:to="link.path">{{link.name}}</router-link>
Copy link
Owner

Choose a reason for hiding this comment

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

Apologies but the escape character has to remain for the template to work. The vue-cli template engine also uses {{}} for variable replacement so the escape character needs to be ignored when a new project is initialised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used this boilerplate for production service last week, and I found some issues and bugs inside the boilerplate.
every change you see now is needed in order make this service work. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was tested on production-ready product, and it works as I used it.
notice that I've changed many things, such as the http-server, and some major configurations in order to make it work, also as a docker container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right.
it was fixed now.

@@ -14,7 +12,7 @@ if (process.env.ENV === 'development' && module.hot) {
// first arguments for `module.hot.accept` and `require` methods have to be static strings
// see https://github.com/webpack/webpack/issues/5668
makeHot(navbarModuleId, navbarComponent,
module.hot.accept('./components/navbar', () => reload(navbarModuleId, (<any>require('./components/navbar')).NavbarComponent)));
module.hot.accept(navbarModuleId, () => reload(navbarModuleId, (<any>require(navbarModuleId)).NavbarComponent)));
Copy link
Owner

Choose a reason for hiding this comment

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

Unfortunately, this genuinely has to be left with the static strings. I wish it were otherwise but Webpack won't work with variables. See webpack/webpack#5668 (comment)

@@ -17,30 +17,33 @@ if (process.env.ENV === 'development' && module.hot) {
// first arguments for `module.hot.accept` and `require` methods have to be static strings
// see https://github.com/webpack/webpack/issues/5668
makeHot(homeModuleId, homeComponent,
module.hot.accept('./components/home', () => reload(homeModuleId, (<any>require('./components/home')).HomeComponent)));
module.hot.accept(homeModuleId, () => reload(homeModuleId, (<any>require(homeModuleId)).HomeComponent)));
Copy link
Owner

Choose a reason for hiding this comment

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

See above


makeHot(aboutModuleId, aboutComponent,
module.hot.accept('./components/about', () => reload(aboutModuleId, (<any>require('./components/about')).AboutComponent)));
module.hot.accept(aboutModuleId, () => reload(aboutModuleId, (<any>require(aboutModuleId)).AboutComponent)));
Copy link
Owner

Choose a reason for hiding this comment

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

See above


makeHot(listModuleId, listComponent,
module.hot.accept('./components/list', () => reload(listModuleId, (<any>require('./components/list')).ListComponent)));
module.hot.accept(listModuleId, () => reload(listModuleId, (<any>require(listModuleId)).ListComponent)));
Copy link
Owner

Choose a reason for hiding this comment

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

See above

@davidmeirlevy
Copy link
Contributor Author

please check again :)

@ducksoupdev
Copy link
Owner

Thanks for rebasing and updating the PR. I have created a template with your branch and unfortunately, I had to make the following changes for it to work as it does currently:

diff --git a/config/webpack.config.module.js b/config/webpack.config.module.js
index f9749ba..becb237 100644
--- a/config/webpack.config.module.js
+++ b/config/webpack.config.module.js
@@ -4,7 +4,7 @@ const
 
 webpackConfig.output = {
   path: helpers.root('/dist'),
-  filename: 'index.js',
+  filename: '[name].js',
   library: '[name]',
   libraryTarget: 'umd',
   umdNamedDefine: true
diff --git a/src/components/navbar/navbar.html b/src/components/navbar/navbar.html
index 0222351..d205ec0 100644
--- a/src/components/navbar/navbar.html
+++ b/src/components/navbar/navbar.html
@@ -15,7 +15,7 @@
     <collapse id="navbar" class="navbar-collapse" v-model="showNavbar">
       <ul class="nav navbar-nav">
         <li v-for="link in links" v-bind:class="{'active' : $route.path == link.path}">
-          <router-link v-bind:to="link.path"></router-link>
+          <router-link v-bind:to="link.path">{{link.name}}</router-link>
         </li>
       </ul>
     </collapse>
diff --git a/src/main.ts b/src/main.ts
index 1c635fc..ded324e 100644
--- a/src/main.ts
+++ b/src/main.ts
@@ -12,7 +12,7 @@ if (process.env.ENV === 'development' && module.hot) {
   // first arguments for `module.hot.accept` and `require` methods have to be static strings
   // see https://github.com/webpack/webpack/issues/5668
   makeHot(navbarModuleId, navbarComponent,
-    module.hot.accept(navbarModuleId, () => reload(navbarModuleId, (<any>require('./components/navbar')).NavbarComponent)));
+    module.hot.accept('./components/navbar', () => reload(navbarModuleId, (<any>require('./components/navbar')).NavbarComponent)));
 }
 
 new Vue({
diff --git a/src/router.ts b/src/router.ts
index 555bdc3..e326acd 100644
--- a/src/router.ts
+++ b/src/router.ts
@@ -17,13 +17,13 @@ if (process.env.ENV === 'development' && module.hot) {
   // first arguments for `module.hot.accept` and `require` methods have to be static strings
   // see https://github.com/webpack/webpack/issues/5668
   makeHot(homeModuleId, homeComponent,
-    module.hot.accept(homeModuleId, () => reload(homeModuleId, (<any>require('./components/home')).HomeComponent)));
+    module.hot.accept('./components/home', () => reload(homeModuleId, (<any>require('./components/home')).HomeComponent)));
 
   makeHot(aboutModuleId, aboutComponent,
-    module.hot.accept(aboutModuleId, () => reload(aboutModuleId, (<any>require('./components/about')).AboutComponent)));
+    module.hot.accept('./components/about', () => reload(aboutModuleId, (<any>require('./components/about')).AboutComponent)));
 
   makeHot(listModuleId, listComponent,
-    module.hot.accept(listModuleId, () => reload(listModuleId, (<any>require('./components/list')).ListComponent)));
+    module.hot.accept('./components/list', () => reload(listModuleId, (<any>require('./components/list')).ListComponent)));
 }
 
 export function createRoutes(prefix: string = ''): RouteConfig[] {
@@ -42,7 +42,6 @@ export function createRoutes(prefix: string = ''): RouteConfig[] {
   ];
 }
 
-
 export const createRouter = () => {
   Vue.use(VueRouter);
   return new VueRouter({mode: 'history', routes: createRoutes()});

I tested it on both Windows and Mac with the same results. I used NodeJS v8.9.3.

Can you let me know what versions of Node you have tested the template with? Also, do you have an idea of why it is not working for me but is for you?

@davidmeirlevy
Copy link
Contributor Author

I'm using node 9.2.0
I'll rebase it again later this week and notify you.

@davidmeirlevy davidmeirlevy force-pushed the master branch 4 times, most recently from 9df5419 to 9a106cb Compare January 23, 2018 13:56
@davidmeirlevy
Copy link
Contributor Author

I uploaded a real project using this template, and it works:
https://github.com/lcurves/front-main
you can see the hot-module replacement..

@davidmeirlevy
Copy link
Contributor Author

@ducksoupdev I solved the conflicts that had with your project and rebased my branch.
you can take it now. :)

@ducksoupdev
Copy link
Owner

Thanks for rebasing the latest :)

Apologies for asking but I am having issues trying to build these new features. If I initialise a new template and run npm run module I get a build error:

image

If I change the config/webpack.config.module.js to the following the build succeeds:

image

However, the package.json points to the dist/index.js and dist/index.d.ts files but they do not get generated:

image

I took a look at your repo https://github.com/lcurves/front-main but none of the dist files exist and I get the same errors as above.

Am I doing something wrong? Could you explain the process of building a module?

@ducksoupdev
Copy link
Owner

Have you seen this issue? - webpack/webpack#4545

I haven't looked at it in depth but it might prevent the module feature from working. We could add an initialisation prompt for the module feature? That way, we can avoid the lazy-loaded components and the module build should work. What do you think?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants