Use Puppeteer for BTR and support statically generating route index files #91
Conversation
…omplete until btr finished
Codecov Report
@@ Coverage Diff @@
## master #91 +/- ##
==========================================
+ Coverage 84.54% 84.72% +0.18%
==========================================
Files 33 33
Lines 1171 1211 +40
Branches 321 332 +11
==========================================
+ Hits 990 1026 +36
- Misses 181 185 +4
Continue to review full report at Codecov.
|
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "extends": "./node_modules/@dojo/scripts/intern/base.json", | |||
| "coverage": [ | |||
| "./dist/dev/src/**/*.js" | |||
| "./dist/dev/src/**/*.js", "!./dist/dev/src/build-time-render/helpers.js" | |||
There was a problem hiding this comment.
Excluding the helpers because cannot instrument code that get's executed in puppeteers browser context.
| }, | ||
| {} as any | ||
| ); | ||
| this._useHistory = useHistory !== undefined ? useHistory : paths.length > 0 && !/^#.*/.test(initialPath); |
There was a problem hiding this comment.
While auto-detecting the use history based on the existence of a # (or lack there of) makes this a smarter module, it would need to be well documented to avoid unexpected behavior. If someone intends to use #, but just forgets to put it at the start of their path, it will pick it up and run as if they meant to use history.
I personally am a fan of having a default (hash), and making any change to that strategy being a deliberate change by the developer in the config file to avoid confusion.
There was a problem hiding this comment.
I didn't want to introduce another option to the .dojorc, we are thinking about supporting routing type via a has flag (feature flag) so this is really to support both before we implement that in cli-build-app. It might change between now and version 5.0.0 but that's fine in the alpha/betas and sniffing the route enables use to use this immediately.
You're right though, if we decide to keep the detection base on # it would have to be documented well. At the moment, without the # build time rendering wouldn't work for hash history (just like routing in the browser).
| return css; | ||
| }, ''); | ||
|
|
||
| html = html.replace(/^(\s*)(\r\n?|\n)/gm, '').trim(); |
There was a problem hiding this comment.
The following would better minify the html
html = html.replace(/(^\s*|\r\n|\n|\t|\s$)/gm, '').trim();JS Fiddle: https://jsfiddle.net/vqn3a6g5/19
Also should this occur after the script and css variables are added in? Since you don't currently the following occurs: https://gist.github.com/KaneFreeman/7cb2991eaa04e516e23e7eb1d720b04d
| setHasFlags | ||
| } from './helpers'; | ||
| const filterCss = require('filter-css'); | ||
| const puppeteer = require('puppeteer'); |
There was a problem hiding this comment.
There is a @types/puppeteer. Letting this be:
import { launch } from 'puppeteer';There was a problem hiding this comment.
@KaneFreeman The types requires me to bump the TS version which I didn't want to do (as it uses conditional types), so I decided against using the types until we purposely bump the TS version.
There was a problem hiding this comment.
@agubler Gotta. Sounds good.
src/build-time-render/helpers.ts
Outdated
|
|
||
| export function generateBasePath(route = '__app_root__') { | ||
| return `<script> | ||
| window.__public_path__ = window.location.href.replace('${route}/', ''); |
There was a problem hiding this comment.
This line is causing the url to be rewritten.
Loading up an App with a default /home route locally (on localhost:9999) causes the browser to redirect to http://localhost:9999/http://localhost:9999/home.
Type: feature
The following has been addressed in the PR:
prettieras per the readme code style guidelinesDescription:
Swaps JSDOM for puppeteer to run BTR and add additional support to statically render index files for
pathswhen using the history API (StateHistory) routing in the application.Resolves #87
Resolves #92