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

Separate build step for NodeBB #5211

Closed
julianlam opened this Issue Nov 16, 2016 · 11 comments

Comments

Projects
None yet
6 participants
@julianlam
Member

julianlam commented Nov 16, 2016

At some point @pitaj is going to go nuts and add webpack into NodeBB, but in the meantime, it may be better practice to split out NodeBB's application starting step from the build/compilation step, so:

./nodebb build and ./nodebb start

Rationale:

  • Allows more robust handling of NodeBB for hosting setups that do not rely on loader.js
  • Allows us to simplify loader.js code and remove all of the code related to passing compiled css/js data back and forth
  • Increases startup speed by removing the largest blocking chunk of code from running during startup (this is technically a cop-out), thereby reducing downtime on forum restarts (this is most definitely not a cop-out)

Steps:

  • Move compilation logic to a separate file (build.js maybe?)
  • Add build step to the end of ./nodebb upgrade and ./nodebb setup
  • Change NodeBB startup so compilation does not take place
  • Deprecate --from-file
  • ./nodebb executable integration
  • Break out meta.css.minify to accept targets (client and acp)
  • Add "rebuild assets" button to ACP dashboard next to "restart"
  • Update documentation for this new update workflow Not necessary as ./nodebb setup and ./nodebb upgrade autobuild. Restart button builds now as well.
  • Update loader.js to not pass in --from-file
  • Update loader.js and cluster logic to no longer pass js/css back and forth (@barisusakli)
  • Add test for build step (@barisusakli)
  • Fix Gruntfile.js (@psychobunny :trollface: )

Bonus points:

  • Warn user if they need to rebuild their assets (e.g. if they activated a plugin or something) -- not quite sure how to detect this at the moment...
  • Consider optimising build step so that plugins and themes do not have to be completely loaded/initialised
  • Consider optimising plugin/theme initialisation so that it no longer needs to parse js/css/tpl files Not worth doing as the benefit is minimal

@julianlam julianlam added this to the 1.3.1 milestone Nov 16, 2016

@julianlam julianlam self-assigned this Nov 16, 2016

@pitaj

This comment has been minimized.

Contributor

pitaj commented Nov 16, 2016

I'd suggest not breaking existing workflows. Maybe have ./nodebb start --prebuilt or something which doesn't build before startup.

On Nov 16, 2016 15:52, "Julian Lam" notifications@github.com wrote:

At some point @pitaj https://github.com/pitaj is going to go nuts and
add webpack into NodeBB, but in the meantime, it may be better practice
to split out NodeBB's application starting step from the build/compilation
step, so:

./nodebb build and ./nodebb start

Rationale:

  • Allows more robust handling of NodeBB for hosting setups that do not
    rely on loader.js
  • Allows us to simplify loader.js code and remove all of the code
    related to passing compiled css/js data back and forth
  • Increases startup speed by removing the largest blocking chunk of
    code from running during startup (this is technically a cop-out), thereby
    reducing downtime on forum restarts (this is most definitely not a
    cop-out)

Steps:

  • Move compilation logic to a separate file (build.js maybe?)
  • Add build step to the end of ./nodebb upgrade and ./nodebb setup
  • Update documentation for this new update workflow


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5211, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAxDdeleEpNEafp5zMDxmTF_zQWxpuJ_ks5q-4k9gaJpZM4K0lHj
.

@julianlam

This comment has been minimized.

Member

julianlam commented Nov 17, 2016

Existing workflows won't break, end result is ./nodebb start would end up being much faster, and setup and upgrade flows will automatically build assets for you.

We would have to add in a button to execute asset building from the ACP, though, as a restart itself won't be enough.

@pichalite

This comment has been minimized.

Contributor

pichalite commented Nov 17, 2016

The new button should probably be "Build & Restart" instead of just "Build Assets". Can't think of a scenario where you would build but not restart. 🤔

@julianlam

This comment has been minimized.

Member

julianlam commented Nov 17, 2016

@pichalite That is a good point. I guess this is a re-introduction of the "reload" button 😆 Except "reload" now "restarts", and "restart" would now "build & restart" :shipit:

julianlam added a commit that referenced this issue Nov 17, 2016

@julianlam

This comment has been minimized.

Member

julianlam commented Nov 17, 2016

@psychobunny When you get to doing the last item, please take a look at 9bf0f6c

@pichalite

This comment has been minimized.

Contributor

pichalite commented Nov 17, 2016

🔜

barisusakli added a commit that referenced this issue Nov 17, 2016

#5211
remove passing js/css between procs

julianlam added a commit that referenced this issue Nov 17, 2016

@lo1tuma

This comment has been minimized.

Contributor

lo1tuma commented Nov 17, 2016

I already really like the direction of this issue 👍 . But I wonder if the rebuild assets functionality is really needed. Without this feature we could make a lot of dependencies devDependencies (e.g. less, postcss, uglify etc) which would result in much smaller docker images. If you would use a custom build process instead of the one nodebb ships then you could even profit from faster install times (npm install --production wouldn’t install stuff like uglify).

Another counter-argument is that the immutable infrastructure approach is on the rise where mutating build artifacts at runtime would be a bad practice.

@julianlam

This comment has been minimized.

Member

julianlam commented Nov 17, 2016

@lo1tuma Thanks for the feedback. A lot of why we do the things we do in NodeBB is because we want to appeal to the lowest (or "lower", in any case) common denominator for admins, in order to reduce barrier to entry.

To that end it does increase the amount of wheel reinvention, but the benefits are somebody new to Node.js (or even just NodeBB) can get up and running with a few simple apt-get and npm i commands and a ./nodebb setup. We want NodeBB to work on as many systems as possible.

If we ended up embracing other build methodologies, we'd end up requiring users pre-configure their servers, install things like grunt, or gulp, custom-tailor a taskrunner file, etc, which make the initial setup much more complex, but also runs the risk of not running properly on different environments.

The absolute worst case scenario would be to have so many pre-requisite programs and configs that we have to maintain a separate docker image in order to make sure everything is "just so".

Re: Immutable infrastructure -- definitely appealing, and is part of why this issue exists. ./nodebb build is a separate "task" from the actual NodeBB app, so you can more easily bundle it into existing build tasks... etc.

At any rate, it's a heck of lot easier to do it now than it was when it was bundled into node app.js 😄

@lo1tuma

This comment has been minimized.

Contributor

lo1tuma commented Nov 17, 2016

@julianlam Thanks for the response. So it seems like my requirements are probably a bit different than those from the rest of the nodebb users. What I need is actually more like a framework (nodebb-framework maybe? 😄 ) than a ready-made application. Basically nodebb without build tasks, cluster-management and pre-installed default plugins/themes.

Also I wouldn’t recommend installing the build tools on the same server where nodebb is running. In many cases build tools have not been implemented with security in mind, so it increases to attack surface on the server. I would recommend to build on a separate machine, preferable a CI/CD pipeline like jenkins.

Anyway, thats out of the scope of this issue. I already appreciate the work that has been done so far 😉 .

barisusakli added a commit that referenced this issue Nov 18, 2016

#5211 run build step before tests
move build to its own file
@julianlam

This comment has been minimized.

Member

julianlam commented Nov 18, 2016

Also I wouldn’t recommend installing the build tools on the same server where nodebb is running. In many cases build tools have not been implemented with security in mind, so it increases to attack surface on the server.

That is a good point to consider. It is important to ensure our build tools (whether they're baked-in or built atop of NodeBB) are securely implemented.

I hope we can both agree that NodeBB would work better with build tools like grunt or gulp after this refactor, than before... I see it this way:

  • Before: NodeBB sidesteps build tools, tries not to affect their operation
  • After: NodeBB allows build tools to better integrate with their operation
@julianlam

This comment has been minimized.

Member

julianlam commented Nov 19, 2016

Consider optimising build step so that plugins and themes do not have to be completely loaded/initialised

While implementing this task (and the other related task), I've discovered that the actual act of loading the css/less/js file paths into memory takes about 6.7msec, which is really not worth the effort for this task:

Consider optimising plugin/theme initialisation so that it no longer needs to parse js/css/tpl files

... unless I already happen to be doing it while resolving another task...

julianlam added a commit that referenced this issue Nov 19, 2016

julianlam added a commit that referenced this issue Nov 22, 2016

Squashed commit of the following:
commit cecb4c0
Author: barisusakli <barisusakli@gmail.com>
Date:   Tue Nov 22 17:21:30 2016 +0300

    some more group tests

commit 4d86262
Author: barisusakli <barisusakli@gmail.com>
Date:   Tue Nov 22 15:55:30 2016 +0300

    use global mod user for flag tests

commit 584cfd0
Author: barisusakli <barisusakli@gmail.com>
Date:   Tue Nov 22 15:29:58 2016 +0300

    suggested topics test

commit 069a90e
Author: barisusakli <barisusakli@gmail.com>
Date:   Tue Nov 22 15:02:28 2016 +0300

    move maintenance mode

commit 0794e39
Author: barisusakli <barisusakli@gmail.com>
Date:   Tue Nov 22 14:57:15 2016 +0300

    fix test

commit 38bc8ad
Author: barisusakli <barisusakli@gmail.com>
Date:   Tue Nov 22 14:37:14 2016 +0300

    maintenance tests

commit 5916530
Author: Julian Lam <julian@nodebb.org>
Date:   Mon Nov 21 11:18:10 2016 -0500

    fix topic creation regression caused by 576df84

commit f0936fc
Author: Julian Lam <julian@nodebb.org>
Date:   Mon Nov 21 11:07:20 2016 -0500

    fixes #5225

commit dcb6773
Author: barisusakli <barisusakli@gmail.com>
Date:   Mon Nov 21 19:03:28 2016 +0300

    #5223

commit 576df84
Author: Julian Lam <julian@nodebb.org>
Date:   Mon Nov 21 10:52:23 2016 -0500

    trimming composer input before doing length check in replies

commit ef6d52c
Author: NodeBB Misty <deploy@nodebb.org>
Date:   Mon Nov 21 09:02:19 2016 -0500

    Latest translations and fallbacks

commit 0a245a8
Author: barisusakli <barisusakli@gmail.com>
Date:   Mon Nov 21 15:44:58 2016 +0300

    tag controller test

commit b49af0a
Author: barisusakli <barisusakli@gmail.com>
Date:   Mon Nov 21 13:49:36 2016 +0300

    #5223

    adjust pagination so each page shows `postsPerPage` posts

commit db14c29
Author: barisusakli <barisusakli@gmail.com>
Date:   Mon Nov 21 13:47:34 2016 +0300

    socket.io/categories tests

commit 6acc79e
Author: barisusakli <barisusakli@gmail.com>
Date:   Mon Nov 21 10:17:45 2016 +0300

    convert title to string

commit 3b40afe
Merge: 0756fcc 2440c74
Author: barisusakli <barisusakli@gmail.com>
Date:   Sun Nov 20 17:50:43 2016 +0300

    Merge branch 'master' of https://github.com/NodeBB/NodeBB

commit 0756fcc
Author: barisusakli <barisusakli@gmail.com>
Date:   Sun Nov 20 17:50:39 2016 +0300

    eslint

commit 2440c74
Author: NodeBB Misty <deploy@nodebb.org>
Date:   Sun Nov 20 09:02:17 2016 -0500

    Latest translations and fallbacks

commit 2c77a88
Author: barisusakli <barisusakli@gmail.com>
Date:   Sun Nov 20 15:11:34 2016 +0300

    closes #5220

commit 51b41a9
Author: barisusakli <barisusakli@gmail.com>
Date:   Sun Nov 20 13:33:35 2016 +0300

    fix eslint

commit 5d4903f
Author: barisusakli <barisusakli@gmail.com>
Date:   Sun Nov 20 03:47:24 2016 +0300

    fix test

commit 6acbd3e
Author: Julian Lam <julian@nodebb.org>
Date:   Sat Nov 19 14:24:37 2016 -0500

    optimising build step for #5211

commit 5e47ea5
Author: NodeBB Misty <deploy@nodebb.org>
Date:   Sat Nov 19 09:02:22 2016 -0500

    Latest translations and fallbacks

commit ef87a2b
Author: barisusakli <barisusakli@gmail.com>
Date:   Sat Nov 19 16:56:37 2016 +0300

    closes #5219

@julianlam julianlam closed this Nov 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment