RC > Allow SPAs to bundle stylesheets that live outside of src/app
#428
Conversation
Codecov Report
@@ Coverage Diff @@
## rc-2.0.0 #428 +/- ##
============================================
+ Coverage 99.3% 99.31% +<.01%
============================================
Files 73 73
Lines 1878 1892 +14
Branches 289 292 +3
============================================
+ Hits 1865 1879 +14
Misses 13 13
Continue to review full report at Codecov.
|
| styles: [ | ||
| '@foo/bar/style.scss', | ||
| 'src/styles/custom.css', | ||
| 'https://google.com/styles.css' |
There was a problem hiding this comment.
I see you filtering out styles that start with http. Should we show an error / warning and mention the externals property?
There was a problem hiding this comment.
Good idea; I'll add that.
|
@Blackbaud-BobbyEarl This is ready for another look! |
lib/sky-pages-module-generator.js
Outdated
| let styles = ''; | ||
| if (cssFilePaths) { | ||
| styles = cssFilePaths | ||
| .filter(path => { |
There was a problem hiding this comment.
No doubt, for most use-cases this array will be very small. I definitely like how readable the map/filter/reduce pattern is, I'm worried about the performance should the array become large. In this case, I think the code could be just as readable using a single for loop. Something like:
let styles = '';
let hasExternal = false;
if (cssFilePaths) {
for (const i = 0, const len = cssFilePaths.length; i < len; i++) {
if (/^http/.test(cssFilePaths[i])) {
hasExternal = true;
} else {
styles += `require('style-loader!${path}');\n`);
}
}
}
If (hasExternal) {
logger.warn(...);
}
This has a few benefits without sacrificing code readability:
- the warning is only shown once despite the number of externals listed (which you could of course still solve using the other pattern).
- Array is only looped through once
I'm happy to discuss further if you disagree @Blackbaud-SteveBrush.
|
This property should also be added to the |
|
@Blackbaud-BobbyEarl Are my changes closer to what you had in mind? |
lib/sky-pages-module-generator.js
Outdated
| @@ -14,6 +16,33 @@ const routeGenerator = require('./sky-pages-route-generator'); | |||
| * @returns {string} source | |||
| */ | |||
| function getSource(skyAppConfig) { | |||
| const cssFilePaths = skyAppConfig.skyux.app && | |||
| skyAppConfig.skyux.app.styles && | |||
| skyAppConfig.skyux.app.styles; | |||
There was a problem hiding this comment.
Is this a typo or missing .length?
I've recently started using https://www.npmjs.com/package/lodash.get to handle this exact scenario in a single line.
const get = require('lodash.get');
const cssFilesPaths = get(skyAppConfig, 'skyux.app.styles', []);
There was a problem hiding this comment.
Good deal! I'll add that.
There was a problem hiding this comment.
You can thank @blackbaud-brandonhare for that tip.
|
@Blackbaud-BobbyEarl Made the required change. I was required to install a new dependency on |
lib/sky-pages-module-generator.js
Outdated
| const cssFilePaths = get(skyAppConfig, 'skyux.app.styles', []); | ||
|
|
||
| let styles = ''; | ||
| const numFiles = cssFilePaths.length; |
There was a problem hiding this comment.
This is non-blocking, but I would've expected this const to be up against the cssFilePaths const, then followed by the let. Just stood out a bit.
skyuxconfig-schema.json
Outdated
| }, | ||
| "styles": { | ||
| "description": "An array of CSS or SCSS files to be bundled with the SPA.", | ||
| "type": "array" |
There was a problem hiding this comment.
I think this also needs the type of items in the array.
"items": {
"type": "string"
}
|
I know @blackbaud-johnly is aware, but tagging him here just as a notification. This would require a documentation entry; however, it's important to note that this work is going in to our |
|
Created https://github.com/blackbaud/skyux2/issues/1822 to document the new configuration properties. |
* RC > Removed all references to SKY UX, changed dependency structure (blackbaud#419) * RC > Updated TSLint rules (blackbaud#422) * Removed legacy omnibar (blackbaud#420) * RC > Config params as an object; always decode URL params (blackbaud#421) * RC > Always provide `envId` regardless of permission scope (blackbaud#427) * RC > Allow SPAs to bundle stylesheets that live outside of `src/app` (blackbaud#428) * RC > Adjusted dev dependencies (blackbaud#429) * RC > Fixed ts-helpers for build (blackbaud#434) * RC > Update from master (blackbaud#425) * RC > Removed global RxJS imports (blackbaud#438) * RC > Replaced error component with iframe (blackbaud#436) * RC > Removed SKY CSS import (blackbaud#443) * RC > Instrument different directory for libraries (blackbaud#448) * RC > Do not ignore Protractor Error 199 (blackbaud#435) * RC > Merged master (blackbaud#444) * RC > Merge master (blackbaud#454) * RC > Upgrade Angular, RxJS, TypeScript (blackbaud#495) * RC > Moved auth-client to peer dependency; fixed `skyux watch` (blackbaud#503) * Disabled webpack host check (blackbaud#505) * Replaced JSHint with ESLint. (blackbaud#506) * RC > Merge master (blackbaud#508) * RC > Changed name of NPM package (blackbaud#501)
Resolves: blackbaud/skyux2#413
Example skyuxconfig.json: