Packaging is really broken

poke commented 6 years ago

The packaging of this module is very broken. There are several issues with it that makes it impossible or at least very difficult to use this module through npm. You can find a more detailed explanation below but there are the following problems with the module:

There are no entry points defined in the package.json.
The bundled file at dist/js/fabric.js creates a local variable fabric which cannot be consumed by a bundler like webpack.
That local variable actually gets declared once for every bundled component.
The typings file declares a namespace fabric while the actual module does not contain another wrapped namespace.

Steps to reproduce

I reproduced this issue using the @microsoft/generator-sharepoint Yeoman generator but these issues exist regardless of that; it just makes it easier to reproduce this quickly.

Create a new project using yo @microsoft/generator-sharepoint. Default options work.
Install dependencies using npm i.
Install this module using npm i office-ui-fabric-js --save-dev

Attempt to import anything from this module in the web part’s code (HelloWorld.ts), for example:

import * as fabric from 'office-ui-fabric-js';
// or to check if it would work with a default import:
// import fabric from 'office-ui-fabric-js';

console.log(fabric);

Notice how this will not compile when running gulp serve:

Error - typescript - src\webparts\helloWorld\HelloWorldWebPart.ts(11,24): error TS2307: Cannot find module 'office-ui-fabric-js'.

Problem 1

The package.json exposes neither a main entry point nor a typings entry point. This makes consuming the library through npm very difficult since there’s no defined entry point. TypeScript, as an example, is unable to compile imports to the office-ui-fabric-js module because of this.

Compare this with for example office-ui-fabric-react, which properly exposes both main and typings.

A simple fix would be to add these properties to the package.json:

"main": "dist/js/fabric.js",
"typings": "dist/js/fabric.d.ts"

Workaround 1

We augment the installed package.json to include entry points, by directly editing node_modules/office-ui-fabric-js/package.json to include those two properties.

This will fix the import, making the code compile. However, when opening the browser, notice how the console.log just logged out an empty object.

Problem 2

The dist/js/fabric.js that now got bundled does not contain any module exports which Webpack could possible combine here. We can see this by looking at the bundle Webpack creates, i.e. dist/hello-world-web-part.js. At the bottom, there should be the webpart module with a line like this:

var fabric = __webpack_require__(1);

But if we look at the webpack module with index 1 in that file, which is the dist/js/fabric.js, we can see that module or exports are never touched by module.

Workaround 2

We can fix this again by augmenting the distributed code, for example by adding the following lines at the bottom of dist/js/fabric.js:

if (typeof(module) !== 'undefined') {
    module.exports = fabric;
}

This makes the console.log from earlier return an object with the fabric contents.

Problem 3

Let’s attempt to actually “use” the module, let’s actually try to use a button: console.log(fabric.Button); will not compile because fabric does not contain Button. According to the type hints, we have to either do:

import * as fabric from 'office-ui-fabric-js';
console.log(fabric.fabric.Button);

or (which effectively means the same though):

import { fabric } from 'office-ui-fabric-js';
console.log(fabric.Button);

If we run this code, we will notice that our browser will throw some cryptic error. This is because fabric.fabric is undefined which we can easily confirm by just doing console.log(fabric.fabric) or console.log(fabric) in the second case.

The problem here is that the fabric.d.ts exposes a namespace fabric although the fabric.js does not.

Workaround 3

The proper fix here would probably be to make fabric.d.ts not expose a namespace. Namespaces, also known as internal modules, are usually not needed since physical files already create (external) modules which is enough of a namespacing construct.

The easier workaround is to adjust the dist/js/fabric.js again to actually expose that namespace:

module.exports.fabric = fabric;

Note that this does not expose the other “visible” members of the module that are exposed by the fabric.d.ts. For example CLOSE_BUTTON_CLASS which is still just a local variable in fabric.js that is thrown away.

In addition, if you look at the dist/js/fabric.js, you can actually see that var fabric appears there a lot of times. So this effectively declares local variables over and over again which might already be a sign that the dist bundling for this package is simply not how it is supposed to be. Ideally, there should only be a single export.

I would expect this to get a lot better if the component modules were actual modules instead of weird things that each declare a namespace.

That way, it would likely also make it possible to consume the module sources without having to consume the bundle. However, I am not involved enough with this to understand how the non-TS files come into play here. Actually, a colleague asked me for help on this which made me look into the bundling process here. But I really hope that this description does help to improve the situation for consumers of the library here.

youranupama commented 6 years ago

I wish i have seen this issue earlier. Could have saved couple of hours. Thanks for posting. Please fix this issue.

vancerver commented 6 years ago

These workarounds seem to work for the most part but since they are all applied to content the dist folder so the changes can get wiped away by NPM.

I am not able to figure out how to apply these workarounds to this repository itself (or rather a fork of it)

poke commented 6 years ago

@vancerver Yes, these are temporary workarounds to get you running somehow. They are not meant as a real fix. I was hoping that the package maintainers would actually do something about this but in the past 5 months they apparently just ignored this.

As I said, I am unfortunately not involved with this project at all (I’m not even using it myself), so I cannot say what exactly would have to change to make this work – if I could, I would probably have submitted a pull request instead. I expect the changes to be somewhat non-trivial though (packaging usually is).

Linda-Editor commented 5 years ago

Microsoft no longer supports this content and will not be responding to bugs or issues. We recommend that you use the newer version, Office UI Fabric, with React as your front-end framework. We are closing this issue; if you still need assistance with Fabric.js, visit Stack Overflow/office-ui-fabric.

OfficeDev / office-ui-fabric-js