Fixed `skyux test/watch` performance #202

Blackbaud-SteveBrush · 2017-06-27T15:52:24Z

Issue is with tslint-loader. (typeCheck is too slow wbuchwalter/tslint-loader#76)
Please confirm that skyux watch --no-coverage still works as intended.

UPDATE 1

Setting typeCheck: false within tslint-loader does make the tests go faster, but it forfeits any of the rules found in tslint.json that require type checking (eg, no-unused-variables). To see a full list of rules affected, visit the Rules documentation and look for rules labeled "Requires Type Info".

The reason it is so slow is because tslint-loader is instantiating a fresh compiler object for every single file it checks. The tslint CLI does not have this problem, however, because it is only generating a single instance of the TypeScript compiler for all files. While there is a PR to fix this problem in tslint-loader it hasn't been touched since May 22.

Options:

We disable type checking in tslint-loader and wait for tslint-loader#78 to be merged, or
Find another method of linting files (such as ts-loader), and forfeit awesome-typescript-loader.
Create a new command skyux lint that spawns ./node_modules/.bin/tslint --type-check --project tsconfig.json, which does the same thing, but would not behave as a webpack loader. We could modify skyux test to also call skyux lint, so that existing build flows would not be affected.

IMO, Option #3 would have the least impact on existing projects, and would not require that much work to accomplish.

UPDATE 2

I ended up writing a webpack loader/plugin that handles TSLint for all webpack files. I initially attempted a Karma preprocessor that would lint all files in src, but it got a little complex due to the way Karma loops through each file one-by-one. Aside from that, this branch will give us feature parity with the rc- branch, will check against all rules in tslint.json, and will execute much faster.

I went down the path of creating a new skyux lint command (as you can see with PR #205), but I feel that it would be better suited for a different feature branch, as the requirements for that command are for ALL files in src, not just the webpack files.

Thoughts, @Blackbaud-BobbyEarl?

Blackbaud-SteveBrush · 2017-06-27T15:56:07Z

config/webpack/test.webpack.config.js

+          use: ['raw-loader', 'sass-loader']
+        },
+        {
+          test: /\.html$/,


Updated CSS, SCSS, and HTML rules to look just like common config (no affect on output).

codecov-io · 2017-06-27T16:08:57Z

Codecov Report

Merging #202 into rc-ng4-upgrade will not change coverage.
The diff coverage is 100%.

@@              Coverage Diff              @@
##           rc-ng4-upgrade   #202   +/-   ##
=============================================
  Coverage             100%   100%           
=============================================
  Files                  39     42    +3     
  Lines                 952    989   +37     
  Branches              146    152    +6     
=============================================
+ Hits                  952    989   +37

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

Impacted Files	Coverage Δ
config/webpack/build.webpack.config.js	`100% <ø> (ø)`	⬆️
config/webpack/serve.webpack.config.js	`100% <ø> (ø)`	⬆️
loader/sky-tslint/program.js	`100% <100%> (ø)`
loader/sky-tslint/index.js	`100% <100%> (ø)`
config/webpack/test.webpack.config.js	`100% <100%> (ø)`	⬆️
loader/sky-tslint/checker-plugin.js	`100% <100%> (ø)`
runtime/directives/sky-app-link.directive.ts	`100% <0%> (ø)`	⬆️

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 978813f...b3f9b2e. Read the comment docs.

…kyux-builder into fix-test-performance

Blackbaud-SteveBrush · 2017-06-29T01:54:24Z

config/webpack/serve.webpack.config.js

@@ -121,11 +119,6 @@ function getWebpackConfig(argv, skyPagesConfig) {
    module: {
      rules: [
        {
-          enforce: 'pre',


Removed this rule because it is already listed in "common" config.

Blackbaud-SteveBrush · 2017-06-29T01:55:58Z

config/webpack/test.webpack.config.js

-          test: /\.css$/,
-          loader: 'raw-loader'
+          test: /\.s?css$/,
+          use: ['raw-loader', 'sass-loader']


Just some cleanup to match what's in common config.

Blackbaud-BobbyEarl · 2017-06-29T17:23:07Z

@Blackbaud-SteveBrush and @Blackbaud-PaulCrowder... I'm thinking the long-term solution to this problem (including fixing blackbaud/skyux2#411) may take slightly longer than anticipated.

Any objections on creating a release immediately after either A) accepting this PR or B) creating a new PR that disables type-checking (knowing that's reducing features), but buys us some time to make the right decision.

Blackbaud-SteveBrush · 2017-06-29T17:56:09Z

I'm obviously partial to merging this in as-is, but I will say that I kept the functionality isolated to the loaders/sky-tslint folder, so if we need to eliminate it for something better in the future, it wouldn't be too difficult.

Also, this branch would not have a drop in features for existing users; another plus.

* Update angular (#189) * Updated dependencies * Updated skyux2 version * Updated dependencies * Using template branch for testing * Reverted protractor config * Pointing to dev branch for local testing * Updated package json * Using explicit types * Updated version of skyux * Updated dependencies * Fixed linting errors * Added fontfaceobserver to types * Fixed tslint errors * Fixed tslint errors * Ensure that the files are not directories * Update tests for file processor * Update plugin-file-processor.js * Update plugin-file-processor.spec.js * Updated version of skyux * Updated config * Added specific version of firefox * Updated tsconfig * Updating travis version * Updated version of firefox * Updated directconnect * Updated protractor * Added firfox launcher * Removed directConnect * Reverting back to directConnect * Removed firefox * Re-added firefox * Accepting insecure certs * Enabled directconnect * use chrome with chromOptions in protractor-dev.conf.js * add sources to travis.yml * add dist: trusty * Removed tsconfig paths * Update typescript-loader * Simpler method for avoiding directories * As ts-node option and fix e2e test * updates for rc.0 release (#190) * Omnibar config (#193) * Using omnibarConfigMap for envid/svcid * Fixing implicit any * Fixed codelyzer path (#192) * Fixed codelyzer path * Added node_modules to excludes * Readded node_modules * Ignore public directory when generating components (#187) * Added ignore pattern to component generator * Component generator should ignore public folder * Updated ignore pattern * Plugin File Processor should not check directories (#186) * Do not check directories * Added nodir option * Updated CHANGELOG.md and package.json for 1.0.0-rc.1 (#194) * Updated template branch, bug fix for component pattern (#195) * Updated branch to master * Fixed bug with ignore components pattern * Updated unit tests * Release 1.0.0 rc.2 (#196) * Update CHANGELOG.md * Update package.json * Remove extra s. (#197) * Fixed type error, updated SKY UX (#199) * Fixed type error * Updated skyux * Updated skyux * Release 1.0.0 rc.3 (#200) * Fixed type error * Updated skyux * Updated skyux * Update package.json * Update CHANGELOG.md * Param functionality (#201) * Fixed typo in OmnibarConfigMap. Allowing querystring params to be case insensitive * Test cleanup * Removed unnecessary mapping functionality * Fixed `skyux test/watch` performance (#202) * Removed tslint-loader * Created custom sky-tslint loader * Release 1.0.0-rc.4 (#207) * Updated changelog, version * Update CHANGELOG.md * Update CHANGELOG.md * Added to skyux builder (#204) * Updated package dependencies (#208) * Updated package dependencies * Update skyux version * use appropriate template branch * Add hash routing option for easy mode. (#206) * Updates rc5 (#209) * Updates for rc.5 * Capitalize Angular. (#211) * Wrote failing test. Then made config property public for template (#212)

* Update angular (#189) * Updated dependencies * Updated skyux2 version * Updated dependencies * Using template branch for testing * Reverted protractor config * Pointing to dev branch for local testing * Updated package json * Using explicit types * Updated version of skyux * Updated dependencies * Fixed linting errors * Added fontfaceobserver to types * Fixed tslint errors * Fixed tslint errors * Ensure that the files are not directories * Update tests for file processor * Update plugin-file-processor.js * Update plugin-file-processor.spec.js * Updated version of skyux * Updated config * Added specific version of firefox * Updated tsconfig * Updating travis version * Updated version of firefox * Updated directconnect * Updated protractor * Added firfox launcher * Removed directConnect * Reverting back to directConnect * Removed firefox * Re-added firefox * Accepting insecure certs * Enabled directconnect * use chrome with chromOptions in protractor-dev.conf.js * add sources to travis.yml * add dist: trusty * Removed tsconfig paths * Update typescript-loader * Simpler method for avoiding directories * As ts-node option and fix e2e test * updates for rc.0 release (#190) * Omnibar config (#193) * Using omnibarConfigMap for envid/svcid * Fixing implicit any * Fixed codelyzer path (#192) * Fixed codelyzer path * Added node_modules to excludes * Readded node_modules * Ignore public directory when generating components (#187) * Added ignore pattern to component generator * Component generator should ignore public folder * Updated ignore pattern * Plugin File Processor should not check directories (#186) * Do not check directories * Added nodir option * Updated CHANGELOG.md and package.json for 1.0.0-rc.1 (#194) * Updated template branch, bug fix for component pattern (#195) * Updated branch to master * Fixed bug with ignore components pattern * Updated unit tests * Release 1.0.0 rc.2 (#196) * Update CHANGELOG.md * Update package.json * Remove extra s. (#197) * Fixed type error, updated SKY UX (#199) * Fixed type error * Updated skyux * Updated skyux * Release 1.0.0 rc.3 (#200) * Fixed type error * Updated skyux * Updated skyux * Update package.json * Update CHANGELOG.md * Param functionality (#201) * Fixed typo in OmnibarConfigMap. Allowing querystring params to be case insensitive * Test cleanup * Removed unnecessary mapping functionality * Added lint command * Fixed `skyux test/watch` performance (#202) * Removed tslint-loader * Created custom sky-tslint loader * Release 1.0.0-rc.4 (#207) * Updated changelog, version * Update CHANGELOG.md * Update CHANGELOG.md * Added to skyux builder (#204) * Updated package dependencies (#208) * Updated package dependencies * Update skyux version * use appropriate template branch * Add hash routing option for easy mode. (#206) * Updates rc5 (#209) * Updates for rc.5 * Capitalize Angular. (#211) * Wrote failing test. Then made config property public for template (#212) * Added skyux lint files * Update CHANGELOG.md * Updated unit tests * Updated unit tests * Fixed e2e test * Fixed e2e tests * Update build.js * Added colors to logger * Updated linter result * Added error log after karma reporter * Update test.js * Added errors to linter result

Blackbaud-SteveBrush added 2 commits June 27, 2017 11:43

Removed tslint-loader

1ac892f

Updated gitignore

7d9b496

Blackbaud-SteveBrush commented Jun 27, 2017

View reviewed changes

Blackbaud-SteveBrush requested review from Blackbaud-PaulCrowder and Blackbaud-BobbyEarl June 27, 2017 16:17

Blackbaud-SteveBrush added 10 commits June 27, 2017 16:01

Re-added tslint-loader

2bfdb7b

Update .gitignore

2a27ebe

Added todo comment

720d925

Merge branch 'fix-test-performance' of https://github.com/blackbaud/s…

f6bc66f

…kyux-builder into fix-test-performance

Cleanup

5eecf55

Removed tslint loader

ec7181d

Created tslint webpack loader and plugin

5976cc6

Added comments

d3fc8e8

Moved imports

f7eb0bf

Removed lint command

700ddab

Blackbaud-SteveBrush commented Jun 29, 2017

View reviewed changes

Blackbaud-SteveBrush added 4 commits June 28, 2017 22:37

Cleaned up implementation

fe97a9b

Updated tests, added mocks

59593f1

Updated unit tests

742cd43

Updated unit tests

b3f9b2e

Blackbaud-SteveBrush closed this Jun 29, 2017

Blackbaud-SteveBrush reopened this Jun 29, 2017

Blackbaud-BobbyEarl approved these changes Jun 29, 2017

View reviewed changes

Blackbaud-SteveBrush merged commit 4272474 into rc-ng4-upgrade Jun 29, 2017

Blackbaud-SteveBrush deleted the fix-test-performance branch June 29, 2017 18:16

Blackbaud-DiHuynh pushed a commit to Blackbaud-DiHuynh/skyux-builder that referenced this pull request Jul 10, 2020

Sending all of hostConfig. (blackbaud#202)

3907f09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed `skyux test/watch` performance #202

Fixed `skyux test/watch` performance #202

Blackbaud-SteveBrush commented Jun 27, 2017 •

edited

Loading

Blackbaud-SteveBrush Jun 27, 2017

codecov-io commented Jun 27, 2017 •

edited

Loading

Blackbaud-SteveBrush Jun 29, 2017

Blackbaud-SteveBrush Jun 29, 2017

Blackbaud-BobbyEarl commented Jun 29, 2017

Blackbaud-SteveBrush commented Jun 29, 2017 •

edited

Loading

Fixed skyux test/watch performance #202

Fixed skyux test/watch performance #202

Conversation

Blackbaud-SteveBrush commented Jun 27, 2017 • edited Loading

UPDATE 1

UPDATE 2

Blackbaud-SteveBrush Jun 27, 2017

Choose a reason for hiding this comment

codecov-io commented Jun 27, 2017 • edited Loading

Codecov Report

Blackbaud-SteveBrush Jun 29, 2017

Choose a reason for hiding this comment

Blackbaud-SteveBrush Jun 29, 2017

Choose a reason for hiding this comment

Blackbaud-BobbyEarl commented Jun 29, 2017

Blackbaud-SteveBrush commented Jun 29, 2017 • edited Loading

Fixed `skyux test/watch` performance #202

Fixed `skyux test/watch` performance #202

Blackbaud-SteveBrush commented Jun 27, 2017 •

edited

Loading

codecov-io commented Jun 27, 2017 •

edited

Loading

Blackbaud-SteveBrush commented Jun 29, 2017 •

edited

Loading