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

Refactoring suggestions #3

Open
8 of 10 tasks
ForbesLindesay opened this issue Aug 27, 2016 · 3 comments
Open
8 of 10 tasks

Refactoring suggestions #3

ForbesLindesay opened this issue Aug 27, 2016 · 3 comments

Comments

@ForbesLindesay
Copy link
Member

ForbesLindesay commented Aug 27, 2016

We need to load content from https://github.com/pugjs/pug-{language}/ when it's not available locally. This is what will happen in the travis deployment. I suggest a few refactorings:

  • have the markdown module actually just render markdown
    • pull file reading/lookup out of the markdown module
    • pull rendering pug template out of the markdown module
    • pull loading strings out of the markdown module
  • Don't generate browserify bundles on the fly (https://github.com/pugjs/pug-www/blob/master/src/index.js#L23-L32) as this is really slow even for development. I'm not even sure if we need this. By writing it this way, the bundle gets built once for each request. In addition to this, each time you build it you are setting up a file watcher to rebuild every time the bundle changes.
  • serve the default language (i.e. en) with no /en/ url prefix
  • do not force browserify into development mode. This is probably what keeps the server alive after calling server.close(). We can manually disable minification as appropriate, but should do that separately.
  • use somewhat fewer promises. File system operations do not need to be async because this will be static once it's hosted. On the other hand, many of our file system operations will turn into reads from GitHub repos, so they will be async anyway.
  • create a clean separation between server and client code
  • support running in more environments by moving away from sass which requires a native module
@TimothyGu
Copy link
Member

TimothyGu commented Aug 27, 2016

  • Don't generate browserify bundles on the fly.
  • do not force browserify into development mode.

Like caching the bundle once it's generated? The reason why I'm forcing development mode is that src/stop.js forces production mode of the entire app, which makes browserify-middleware automatically do a few things like minifying, gzipping, and caching, all of which I don't need for static website generation.

I'm also a bit curious as to what mechanism browserify uses to prevent the process from ending...

  • use somewhat fewer promises

Async functions make me forget I'm using promises sometimes :)

  • create a clean separation between server and client code

I mean, that's pretty much already the case. The exclusively browser-side code is in src/entry/ and src/browser/, and src/components/pug-preview.js is designed to work on both platforms. The only slightly tricky business is with the demos JSON array, but I'm open to suggestions as to how to make React work better on browser-side.

  • support running in more environments by moving away from sass which requires a native module

Unless we decide to move away from Bootstrap (which I used to counter my laziness and lack of talent at CSS), I'm reluctant to discard Sass which does make customizing Bootstrap radically easier. Or of course we can always cheat with the (admittedly gigantic) emscripted version. After all, pug-www is going to be static anyway, so unless the developer is using Windows this shouldn't be a major problem. Or maybe people are. I don't know.

@ForbesLindesay
Copy link
Member Author

what mechanism browserify uses to prevent the process from ending

browserify in development watches the files for changes so it can re-build on the fly. That file watcher keeps the process alive. That's not a deliberate choice, but we need to rebuild on changes in development.

Each of the things browserify-middleware does can be enabled/disabled individually. Gzip and caching is not worth worrying about either way. minifying is the only thing that we might want to disable because stop can do it for you, but doing it twice won't really do any harm.

Unless we decide to move away from Bootstrap

Long term, we could do that. There's no rush on that last part at the moment, but I want to make this as easy as possible for users (including windows users) to contribute to.

@TimothyGu
Copy link
Member

Another thing about node-sass: it provides prebuilt binaries for a wide variety of OS, architectures, and Node.js, so in most cases a compiler toolchain shouldn't be needed.

Eventually it took a bit more than expected to fix the Browserify issue, but it works fine now.

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

2 participants