Sp-dev-docs: Using KnockOut template with containerless element (comments) breaks on production

[shaipetel]

Category

• [ ] Question
• [ ] Typo
• [x] Bug
• [ ] Enhancement / Suggestion
• [ ] Additional article idea

Expected or Desired Behavior

When I use KnockOut and build templates that use containerless elements, KnockOut puts these in HTML comments.
For example, html file that has this in the template:
< !-- ko if: source.title() === '' --> Please configure your data source by editing this web part properties. < !-- /ko --> ... more html here ...
And in my code I use "require" to load it.
This works well during dev, I expect it to also work when preparing to deploy to CDN.

Observed Behavior

When using gulp --ship to build my code for the CDN, the bundled JS file removes all JS/HTML comments.
This removes all KnockOut comments as well, breaking all my templates.

Steps to Reproduce

Open a simple KnockOut web part project.
In the KnockOut template html file, add any valid expression in a ko comment (containerless ko expression).
run it using gulp serve <= it works.
use gulp --ship to get the files into your CDN, run it from there <= doesn't work.
You can examine the bundle file as well and you will see that all the KO comments have been removed from the HTML.

This is a major issue, as it basically breaks the promise that we can use KnockOut to build SPFx web parts.
Comments are an important integral part of KnockOut templates and stripping all comments without giving the developer an option to preserve them basically blocks us from releasing our web part.

[shaipetel]

Info from the good guys @mstf, this issue appears to be caused by 'html-loader' plugin, and should be replaced with 'knockout-html-loader' instead.
I am still verifying how to apply the fix exactly and that it works. Once I do I will post the instructions here.

[waldekmastykarz]

@shaipetel most likely you will need to change the loader in the webpack config as described @ https://dev.office.com/sharepoint/docs/spfx/toolchain/extending-webpack-in-build-pipeline.

[shaipetel]

Here is the change:

1. Add this npm package to your package.json:
   "knockout-html-loader": "latest"
   I added it to devDependencies

1. Run "npm install" to get this package

2. edit your gulpfile.js and replace "html-loader" loader with "knockout-html".
   My final gulpfile.js looks like this:

'use strict';

const gulp = require('gulp');
const gutil = require('gulp-util');
const build = require('@microsoft/sp-build-web');

build.configureWebpack.mergeConfig({
    additionalConfiguration: (config) => {
        config.module.loaders.forEach((loader)=>{
            if( loader.loader === "html-loader"){
                gutil.log("replacing html-loader for knockout-html to preserve knockout comments");
                loader.loader = "knockout-html";
            }
        });
        return config;
    }
});

build.initialize(gulp);

Now, when building using --ship parameter, you will see all your knockout html templates go into a script instead of a string, and all comments are broken into separate elements.

I can confirm this works for me.

Thanks @pgonzal @iclanton

[shaipetel]

Thanks @waldekmastykarz I know how / where to do it, I just needed some time to find the most efficient way with least amount of code.
Here is what I got, is there a better way?
I posted my solution in case anyone else needs it.

[octogonz]

@iclanton @nickpape-msft @patmill to consider whether we should potentially support this directly from the tools, given that Knockout is currently one of our Yeoman scenarios.

[iclanton]

We should. I'll create a task for it.

[waldekmastykarz]

@shaipetel I'd suggest you don't use latest as version indicator for the KO loader or any other package for that matter. This makes your solution very prone to changes which are hard to track because they depend on the particular moment in time when dependencies have been restored.

[shaipetel]

@waldekmastykarz good point, for that matter I have another question then:
How reliable is the existence of an npm package in a specific version?
Should I be worried that in maybe 5 months, some packages I use (not the ones Microsoft use) will be gone and I won't be able to get it from npm?
I mean, is there some sort of SLA they keep or a package can be pulled and gone without notice?

In short, should I be keeping a copy of all packages I use in my source control?

Thanks!

[waldekmastykarz]

If you want to be really sure of their integrity, the short answer is yes. Without it, there a number of things you have to take into account, like authors of the different packages not using semver correctly and releasing backwards incompatible code in minor releases. Npmjs.com doesn't allow overwriting existing versions of packages or deleting packages that have been published for longer than 24h (if I recall it correctly). A package can be unpublished (deprecated) but not deleted. But if you're using another registry, it might have its own rules. At the end of the day it's your code that's at stake, so if you want to be really sure of the contents of your dependencies, getting a local copy of them is definitely an option worth considering.

[octogonz]

A package can be unpublished (deprecated) but not deleted.

Is that true? I was pretty sure that "npm unpublish" permanently blocks people from using "npm install" to obtain that release.

[shaipetel]

I couldn't find any info on it it is all very vague that's why I asked. I'm just wondering if it is smart to exclude all packages from my source control if packages can go missing in the future.

Currently spfx project is set up not to check in any packages, which is a smart move if there would be some guarantees that my packages would always be available.

[shaipetel]

Too soon to be happy...
I just noticed "knockout-html" loader works in most cases but is full of bugs.
Some of my templates won't load correctly, it seems text without surrounding element is simply removed in some cases.

Also, it returns an object array instead of a simple string.

Seems this issue should still be active.

Question: the built in html-loader that you use have a config flag to skip removing comments:
from their documentation:
module: { rules: [{ test: /\.html$/, use: [ { loader: 'html-loader', options: { minimize: true, removeComments: false, collapseWhitespace: false } }], }] }

I couldn't find a way to send this configuration option using the SPFx gulpfile, since it loads them using "loaders" and not "rules", and it ignores my options.
Anyone have a clue how to specify this flag in SPFx? Any help would be very appreciated.
I think there is a way to send it directly in the require(...) statement, i will explore that option for now.

[shaipetel]

Ok, it was easier than I thought.

No need for any other modules/packages. Here is the fixed code for the gulpfile.js that makes sure no HTML comments are removed:

'use strict';

const gulp = require('gulp');
const gutil = require('gulp-util');
const build = require('@microsoft/sp-build-web');

build.configureWebpack.mergeConfig({
    additionalConfiguration: (config) => {
        config.module.loaders.forEach((loader) => {
            if (loader.loader === "html-loader") {
                loader.loader += "?removeComments=false";
            }
        });
        return config;
    }
});

build.initialize(gulp);

[waldekmastykarz]

@pgonzal yes and no. When you try to install the unpublished package, the installation completes with a warning that the package has been abandoned. Nevertheless the package gets installed in your project. If you look at the contents of the installed package you won't find any of the files. Instead you'll see a readme.md with a following note:

Deprecated Package

This package is no longer supported and has been deprecated. To avoid malicious use, npm is hanging on to the package name.

Please contact [email protected] if you have questions about this package.

[shaipetel]

@waldekmastykarz holy guacamole! You mean you go through the npm install, it appears your package was installed, but instead you get an empty folder?
Take a look at the modules folder, there are hundreds of them under there - would be very hard to identify if one of them pulls this off... I guess i will be backing up all the packages that I add myself. I think MSFT should make this very clear to people using SPFx.

[octogonz]

Their design choice sort of makes sense to me: Since NPM packages are open source, unpublishing a package is extremely rare. Usually it's because there is something wrong with the release, e.g. a serious security vulnerability, licensing infringement, etc. In the past, NPM allowed you to completely delete a package or version, but this meant someone could republish completely different content using the exact same name/version. That was a security concern, and it defeated the guarantee of shrinkwrap, and also it wasn't handled correctly by caches.

As a compromise, I'm guessing they changed the model so that when you "unpublish", it just replaces the release with an empty token package. That way the rest of your packages still install, and they have a mechanism for issuing a warning.

You are right that npmjs.com is mutable. Existing content can disappear, which can break old projects, even if you are using shrinkwrap. There was one famous story where a person became upset and maliciously unpublished all their stuff. Casual developers don't worry too much about this, since it's just one of many very unlikely things that isn't worth fretting about. For larger enterprises, you can protect yourself by setting up a caching NPM proxy, which also allows you to review/restrict the packages that your developers are installing.

[msft-github-bot]

Issues that have been closed & had no follow-up activity for at least 7 days are automatically locked. Please refer to our wiki for more details, including how to remediate this action if you feel this was done prematurely or in error: Issue List: Our approach to locked issues