Serve and open built files #219

Blackbaud-BobbyEarl · 2017-07-13T15:07:33Z

This modification exposes the -l or --launch flag to skyux build, which will accept local and host as values. Essentially, it easily allows a "built" SPA to be served.

blackbaud/skyux2#562
blackbaud/skyux2#890

… serve-build

codecov-io · 2017-07-14T17:40:47Z

Codecov Report

Merging #219 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #219   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          56     58    +2     
  Lines        1305   1340   +35     
  Branches      195    197    +2     
=====================================
+ Hits         1305   1340   +35

Flag	Coverage Δ
#builder	`100% <100%> (ø)`	⬆️
#runtime	`100% <ø> (ø)`	⬆️
#srcapp	`100% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
cli/build.js	`100% <100%> (ø)`	⬆️
config/webpack/serve.webpack.config.js	`100% <100%> (ø)`	⬆️
cli/utils/browser.js	`100% <100%> (ø)`
utils/runtime-test-utils.js	`100% <100%> (ø)`	⬆️
cli/e2e.js	`100% <100%> (ø)`	⬆️
loader/sky-pages-module/index.js	`100% <100%> (ø)`	⬆️
lib/sky-pages-module-generator.js	`100% <100%> (ø)`	⬆️
cli/utils/stage-library-ts.js	`100% <100%> (ø)`	⬆️
index.js	`100% <100%> (ø)`	⬆️
cli/utils/server.js	`100% <100%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96960aa...bdc59f5. Read the comment docs.

Blackbaud-BobbyEarl · 2017-07-14T17:45:03Z

cli/utils/stage-library-ts.js

@@ -66,10 +66,10 @@ function getFileContents(filePath) {
  switch (path.extname(filePath)) {
    case '.scss':
      contents = compileSass(filePath);
-      break;
+    break;


It's unclear how this got in to start with, but the linter complained to be about the spacing.

… serve-build

Blackbaud-BobbyEarl · 2017-08-18T12:58:53Z

Hey @Blackbaud-SteveBrush and @Blackbaud-PaulCrowder, this one is ready for review.

The factoring out of server.js and browser.js so they could be shared across different CLI commands, unfortunately meant I had to move quite a bit of the hoisting I would doing for tests to different locations (which is why the PR looks so large).

Let me know if you have any questions or want to talk through any of it.

Blackbaud-SteveBrush · 2017-08-18T18:30:55Z

@Blackbaud-BobbyEarl: Launching on localhost works great!

skyux build --launch local
skyux build -l local

However, when using --launch host, it seems that locales aren't being parsed correctly:

Blackbaud-SteveBrush · 2017-08-18T18:30:58Z

One last comment, and this is just a matter of preference: it might be nice to have a default value, so that consumers could type skyux build -l.

… serve-build

Blackbaud-BobbyEarl · 2017-08-19T12:25:13Z

Thanks for the feedback @Blackbaud-SteveBrush. Based on it, I've implemented the following changes:

Adjusted the flag to --serve or -s. This frees up the --launch flag to be used exactly as it is with skyux serve, accepting none, local, and host (default) as values. This is also handy when running the command more than once, meaning you can skyux build -s the first time, and skyux build -s -l none on subsequent runs.
Fixed the resources/assets not loading when launching host.

These together mean you can run some pretty neat commands, for example to launch the local url in safari... skyux build -s -l local -b safari. Anyway, it's ready for review again.

Blackbaud-BobbyEarl · 2017-08-21T15:06:23Z

cli/e2e.js

@@ -91,6 +85,7 @@ function spawnProtractor(chunks, port, skyPagesConfig) {
    protractorPath,
    [
      getProtractorConfigPath(),
+      `--disableChecks`,


FYI, adding this check to solve the "unknown arguments" warning (soon to be error) that's shown. https://github.com/angular/protractor/blob/master/CHANGELOG.md#features-3

Blackbaud-SteveBrush · 2017-08-21T15:32:39Z

cli/build.js

+  } else if (argv.serve) {
+    buildServe(argv, skyPagesConfig, webpack, isAot);
+  } else {
+    return buildPromise(argv, skyPagesConfig, webpack, isAot);


Curious why we're returning the promise for buildPromise, but not for buildServe? Would it be appropriate to also catch any server errors?

function buildServe(argv, skyPagesConfig, webpack, isAot) { return server.start(skyPagesConfigUtil.getAppBase(skyPagesConfig)) .then(port => { argv.assets = argv.assets || `https://localhost:${port}`; return buildPromise(argv, skyPagesConfig, webpack, isAot); }) .then(stats => browser(argv, skyPagesConfig, stats, port)); }

This also makes me wonder if we were ever handling failures during the build step. Aside from linting errors, I don't see a process.exit for build issues. Is that by design?

If skyux build fails outside of linting, the ProcessExitCode plugin calls process.exit.

I was really only concerned with returning a promise from buildPromise knowing that cli/e2e.js imports build, which wouldn't be passing the serve command. It definitely makes sense to make the signature the same (always returning a promise) regardless of the arguments.

tl;dr I'm going to push an update to always have build return a promise, but process.exit is handled in the webpack plugin.

Gotcha. That makes sense!

Wonder if I should still reject the promise, even if it's linting that fails? I'm thinking so, such that the command always returns a promise regardless of what actually failed.

Blackbaud-SteveBrush · 2017-08-21T15:35:21Z

cli/e2e.js

@@ -218,7 +177,7 @@ function e2e(argv, skyPagesConfig, webpack) {
    return killServers(0);
  }

-  spawnServer()
+  server.start()


So much cleaner; love it.

Blackbaud-SteveBrush · 2017-08-21T15:38:56Z

config/protractor/protractor-dev.conf.js

+      } else {
+
+        const url = 'https://github.com/blackbaud/skyux-template';
+        const branch = 'master';


I pointed the branch to builder-dev (a protected branch) so that we could adjust the template as needed without affecting what consumers are cloning when typing skyux new. Unsure how you feel about this.

https://github.com/blackbaud/skyux-template/tree/builder-dev

(This is also helpful for when we have various versions of SKY UX that need to be tested during a Travis build.)

Changing this wasn't intentional... chalking it up to me being ignorant when resolving merge conflicts.

Learning about it though, I'm torn. It seems nice for development for new features in builder that may require a change to the template. On the other hand, running builder's e2e tests is used to test that skyux new runs as intended for consumers, in which case they'd be using the master branch.

I'll change it back to builder-dev for the sake of this PR and we can talk more about it later.

…h of template

Blackbaud-SteveBrush

This is non-blocking feedback (because the feature works fantastically), but the build promise chain could be simplified a bit since all of the various methods return promises already (removed all new Promise, resolve, reject, etc.):

// ...

function buildServe(argv, skyPagesConfig, webpack, isAot) {
  return server.start(skyPagesConfigUtil.getAppBase(skyPagesConfig))
    .then(port => {
      argv.assets = argv.assets || `https://localhost:${port}`;
      return buildCompiler(argv, skyPagesConfig, webpack, isAot)
        .then(stats => browser(argv, skyPagesConfig, stats, port));
    });
}

function buildCompiler(argv, skyPagesConfig, webpack, isAot) {
  // ...

  return runCompiler(webpack, config, isAot)
    .then(stats => {
      if (isAot) {
        cleanupAot();
      }

      return stats;
    });
}

function build(argv, skyPagesConfig, webpack) {
  // ...
  const name = argv.serve ? buildServe : buildCompiler;
  return name(argv, skyPagesConfig, webpack, isAot);
}

// ...

Blackbaud-BobbyEarl · 2017-08-21T20:34:24Z

Any feedback before this gets merged in @Blackbaud-PaulCrowder?

Bobby Earl added 9 commits July 13, 2017 11:03

Allowing mechanism for serving built files

7bc54be

Fixed merge conflicts from master

a88ab54

Cleaned up bad merge

afae356

Fixed linting errors

c60e9ae

Merge branch 'master' into serve-build

8a8067a

Merge branch 'master' into serve-build

e7101f1

Returning from build. Removing Protractor warning

047b223

Merge branch 'serve-build' of github.com:blackbaud/skyux-builder into…

a21343d

… serve-build

Moved files. Fixed bug causing test failure

d1a54ea

Blackbaud-BobbyEarl requested review from Blackbaud-SteveBrush and Blackbaud-PaulCrowder July 14, 2017 17:40

Blackbaud-BobbyEarl commented Jul 14, 2017

View reviewed changes

Blackbaud-BobbyEarl changed the title ~~Serve and open built files~~ [HOLD] Serve and open built files Jul 14, 2017

Bobby Earl and others added 13 commits July 14, 2017 16:17

Manually setting name if launching after build

8a0808c

Using different server to allow for better redirects

8947d70

Merged conflicts

fb4f88e

Merge branch 'master' into serve-build

8513ffd

Cleanup

8c48663

Fixed conflicts

7b11e75

Cleaned up linting

8fdf1a6

Fixed mock

a7a7781

Merge branch 'master' into serve-build

66f72e9

Cleaned up static directory

2d3cb65

Merge branch 'serve-build' of github.com:blackbaud/skyux-builder into…

c0e743b

… serve-build

Merged master

53dfb1c

Merge branch 'master' into serve-build

503c7d6

Blackbaud-BobbyEarl changed the title ~~[HOLD] Serve and open built files~~ Serve and open built files Aug 18, 2017

Merge branch 'master' into serve-build

7d418a5

Bobby Earl added 2 commits August 18, 2017 14:11

Removed unnecessary forward slash. Fixed logging.

ced2832

Merge branch 'master' into serve-build

fd07c81

Bobby Earl added 3 commits August 18, 2017 16:32

Separated launch from serve functionality. Defaulting launch to host

dc29453

Merge branch 'serve-build' of github.com:blackbaud/skyux-builder into…

70e7479

… serve-build

Refactored to use --serve and correctly set assets

2085ebb

Blackbaud-BobbyEarl commented Aug 21, 2017

View reviewed changes

Blackbaud-SteveBrush reviewed Aug 21, 2017

View reviewed changes

Always returning promise (if lint passes) from build. Fixed e2e branc…

7c6c298

…h of template

Blackbaud-SteveBrush approved these changes Aug 21, 2017

View reviewed changes

Cleaned up promises

bdc59f5

Blackbaud-SteveBrush approved these changes Aug 21, 2017

View reviewed changes

Blackbaud-BobbyEarl merged commit 37878bb into master Aug 23, 2017

Blackbaud-BobbyEarl deleted the serve-build branch August 23, 2017 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serve and open built files #219

Serve and open built files #219

Blackbaud-BobbyEarl commented Jul 13, 2017 •

edited

Loading

codecov-io commented Jul 14, 2017 •

edited

Loading

Blackbaud-BobbyEarl Jul 14, 2017

Blackbaud-BobbyEarl commented Aug 18, 2017

Blackbaud-SteveBrush commented Aug 18, 2017

Blackbaud-SteveBrush commented Aug 18, 2017

Blackbaud-BobbyEarl commented Aug 19, 2017

Blackbaud-BobbyEarl Aug 21, 2017

Blackbaud-SteveBrush Aug 21, 2017

Blackbaud-SteveBrush Aug 21, 2017

Blackbaud-BobbyEarl Aug 21, 2017

Blackbaud-SteveBrush Aug 21, 2017

Blackbaud-BobbyEarl Aug 21, 2017 •

edited

Loading

Blackbaud-SteveBrush Aug 21, 2017

Blackbaud-SteveBrush Aug 21, 2017

Blackbaud-SteveBrush Aug 21, 2017

Blackbaud-SteveBrush Aug 21, 2017

Blackbaud-BobbyEarl Aug 21, 2017 •

edited

Loading

Blackbaud-SteveBrush left a comment

Blackbaud-BobbyEarl commented Aug 21, 2017

Serve and open built files #219

Serve and open built files #219

Conversation

Blackbaud-BobbyEarl commented Jul 13, 2017 • edited Loading

codecov-io commented Jul 14, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Blackbaud-BobbyEarl commented Aug 18, 2017

Blackbaud-SteveBrush commented Aug 18, 2017

Blackbaud-SteveBrush commented Aug 18, 2017

Blackbaud-BobbyEarl commented Aug 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blackbaud-BobbyEarl Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blackbaud-BobbyEarl Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

Blackbaud-BobbyEarl commented Aug 21, 2017

Blackbaud-BobbyEarl commented Jul 13, 2017 •

edited

Loading

codecov-io commented Jul 14, 2017 •

edited

Loading

Blackbaud-BobbyEarl Aug 21, 2017 •

edited

Loading

Blackbaud-BobbyEarl Aug 21, 2017 •

edited

Loading