Material-UI, which depends on jss fails (aka: "browser" fields semantics)

[phillipsheridan]

Using MUI npm package with react portlet generated from yeoman liferay-js generator gives error: The resource from “http://localhost:8085/o/js/resolved-module/@test-original$material-ui/core@4.1.3/.js?languageId=en_US” was blocked due to MIME type (“text/html”) mismatch (X-Content-Type-Options: nosniff).

I believe the error is just because browser couldn't find what it was looking for.

@izaera Here is the zip file of the project. I removed the node_modules so make sure to do an npm i and then also update the liferay location.

test-original.zip

[izaera]

Revisiting this, I have deployed the project with version 2.13.1 and it appears that your problems may be caused by the jss package that is bundling modules with ES6+ syntax.

For example, the module jss@10.0.0-alpha.24/dist/jss.cjs.js finishes with:

  export default index;
  export { hasCSSTOMSupport, create, getDynamicStyles, toCssValue, createRule, SheetsRegistry, SheetsManager, RuleList, sheets, createGenerateId };

which is not supported by the liferay-npm-bundler as it requires CommonJS ES5 syntax.

We have a workaround demonstrated in this project's configuration file.

As you can see, we are redefining the plugins to be applied for jss-* packages. In particular, we are removing the liferay-npm-bundler-plugin-replace-browser-modules plugin from the build so that the browser headers in jss's package.json are not taken into account. This is because those headers are "incorrect" and break the build.

To dig deeper in the "incorrectness" of such headers, there's a semantic problem with them. On the one hand the "browser" header may be used to provide polyfills for server side modules, but it can also be used to provide optimized versions of modules for bundlers (usually targeting webpack with a vendor recommended configuration alone).

We have usually seen case 1 with the bundler where people provide polyfills for node-only modules so that they can also be used in the browser, but in this case, we are facing case number 2, i.e., jss provides a browser header to try to optimize the final webpack bundle.

Because we (the bundler) cannot know what browser headers are meant to polyfill and which ones are meant to optimize the webpack build, we may not fix this automatically.

However, I will turn this into an issue to explore the possibility of making this type of issues easier to diagnose and fix. Ideas that come to my mind are:

1. Maintain a list of package like jss and automagically try to "fix" them
2. Provide a configuration switch in the replace-browser-modules plugin
3. Add diagnostic code to detect ES6+ modules in the output JAR and warn about them
4. Leverage loaders to compile ES6+ modules in third party packages on the fly

[izaera]

https://issues.liferay.com/browse/IFI-1071

[izaera]

Seems like we should drop support for 'unpkg' field introduced in #43 (as per https://github.com/mjackson/unpkg/issues/63)

And we should do the same for 'jsdelivr' field too.

Apparently webpack uses the browser field alone as default -> https://webpack.js.org/configuration/resolve/#resolvealiasfields

We will go with browser field by default and will let users configure the aliasFields in the global config section of .npmbundlerrc so that it can be easily overriden.

[izaera]

This is the "spec" for the browser field -> https://github.com/defunctzombie/package-browser-field-spec

[izaera]

As this is related to #363, we will try to fix both issues in the same PR.

[izaera]

OK, so JSS ships with different flavors (see https://cssinjs.org/cdn?v=v10.0.0):

1. jss.js -> UMD (uses define()/module.exports)
2. jss.cjs.js -> CommonJS ES5 (uses module.exports)
3. jss.esm.js -> ESM module (uses ES6+ export)

In its package.json:

1. main points to dist/jss.cjs.js
2. module points to dist/jss.esm.js
3. unpkg points to dist/jss.bundle.js

Because we are paying attention to unpkg as if it was a browser field, we end up bundling dist/jss.bundle.js which is using ES6+ export keyword.

So this bug should be fixed by just avoiding the unpkg field :+1:

[izaera]

I've been doing all type of tests with webpack and reviewing code to grab a deep knowledge of how the browser field works.

Useful resources:

• Node resolution algorithm: https://nodejs.org/api/modules.html#modules_all_together
• Webpack resolution package: https://github.com/webpack/enhanced-resolve

Unfortunately, the browser field semantics does not seem compatible with AMD unless we rewrite require() calls.

I'll show an example, but there are more use cases where this behaviour appears.

Given the following fs hierarchy and contents inside a project:

inner-override/
inner-override/index.js
inner-override/browser
inner-override/browser/replaced.js
inner-override/browser/is-object.js
inner-override/inner
inner-override/inner/index.js
inner-override/inner/browser
inner-override/inner/browser/replaced.js
inner-override/inner/browser/is-object.js
inner-override/inner/package.json
    "browser": {
        "is-object": "./browser/is-object.js",
        "./replaced.js": "./browser/replaced.js"
    }
inner-override/package.json
    "browser": {
        "is-object": "./browser/is-object.js",
        "./inner/replaced.js": "./browser/replaced.js"
    }

CASE 1

Look at the replacement of the ...replaced.js module. It appears twice in the inner-override folder's package.json and in the inner-override/inner folder's package.json and each maps to a different file.

This works perfectly in webpack because:

• When inner-override requests the ./inner/replaced.js file it gets inner-override/browser/replaced.js
• When inner-override/inner requests the ./replaced.js file it gets inner-override/inner/browser/replaced.js

However, under AMD, both requires are the same because both map to the canonical inner-override/inner/replaced module name and we can only have one code for that module so, either inner-override's call fails or inner-override/inner does.

The only way to overcome this limitation is to rewrite the requires of inner-override files and do the same for inner-override/inner files pointing them to the proper module in each case.

This would work for the package internally but, would it work for external packages that require inner-override/inner/replaced in this one? Further tests need to be done to see what webpack does and see what do we exactly need to put under that canonical module name.

CASE 2

Now look at the replacement of is-object. It is replaced two times again, but in this case webpack behaves differently:

1. When inner-override requests is-object it gets inner-override/browser/is-object.js.
2. When inner-override requests ./inner/is-object it gets inner-override/inner/browser/is-object.js.
3. When inner-override/inner requests is-object it gets inner-override/inner/browser/is-object.js.
4. When inner-override/inner requests ../is-object it gets inner-override/inner/browser/is-object.js too.

Look at 4. It's very unexpected and again causes the same problem as before: the canonical module inner-override/is-object is resolved to two different things in different contexts. This implies that we need rewriting require() calls again.

The mistery of the unexpected behaviour in 4 is easily explained by adding a require('../replaced.js') call in inner-override/inner. That makes webpack fail (because there's no inner-override/replaced.js file in the project).

That suggests that webpack doesn't analyze the browser fields of folders outside of the current one and its children. Further tests should be done to see if this hypothesis holds when folders are not in a direct relationship (for example: sibling-sibling instead of parent-child).

[izaera]

I made the test of requesting replaced from outside. I added a sibling folder side by side with inner-override and required ../inner-override/inner/replaced.js from it.

Webpack returns inner-override/inner/browser/replaced.js so it looks like the usual behaviour is that children override parents unless the parent has an explicit override of the children in its package.json.

What a cool feature :unamused:

Note that this case can be overcome in AMD by rewriting requires in the parent that overrides the child and simply redirecting the canonical module for the rest of people.

[izaera]

When doing the same for is-object (requesting it from sibling) a consistent behaviour is expected:

1. require('../inner-override/is-object') returns inner-override/browser/is-object.js
2. require('../inner-override/inner/is-object') returns inner-override/inner/browser/is-object.js

This leaves me wondering why CASE 2.4 holds :thinking: . The only difference I see is that in this case sibling doesn't define is-object in browser, so there's no uncertainty on what should be resolved in each case.

[izaera]

If I make this new test:

parent-package/
parent-package/browser
parent-package/browser/is-object.js
parent-package/inner
parent-package/inner/deep
parent-package/inner/deep/index.js
parent-package/package.json
    "browser": {
        "is-object": "./browser/is-object.js"
    }

And I require from parent-package/inner/deep/index.js:

1. is-object: the file parent-package/browser/is-object.js is returned
2. ../is-object: fails because it doesn't exist
3. ../../is-object: works and returns parent-package/browser/is-object.js

So, the weirdness in CASE 2.4 is explained because ../is-object is not the is-object package in the .. context but the true local ../is-object module, which exists because it's in the browser field.

Thus, the browser field is a bit tricky because in this case it serves two different purposes:

1. Internally, it is used by the folder and its children to alias a real package
2. Externally it can be interpreted as a "virtual" module inside that folder (that explains why 2.4, which is not so unexpected in the end :sweat_smile: .