bootsharp icon indicating copy to clipboard operation
bootsharp copied to clipboard

Support ES module import in browser

Open DanielHabenicht opened this issue 2 years ago • 58 comments

Hi again,

I tested the library with angular and it is running into an error:

universalModuleDefinition:1 Uncaught ReferenceError: global is not defined
    at universalModuleDefinition:1:1
    at Object.9424 (universalModuleDefinition:1:1)
    at __webpack_require__ (bootstrap:19:1)
    at Module.1896 (home.component.html:14:263)
    at __webpack_require__ (bootstrap:19:1)
    at Module.721 (universalModuleDefinition:1:1)
    at __webpack_require__ (bootstrap:19:1)
    at Module.23 (app.component.html:6:8)
    at __webpack_require__ (bootstrap:19:1)
    at Module.8835 (environment.ts:16:71)

Which is expected as the angular team came to the conclusion to not support global in their webpack build for broad compatibility. https://github.com/angular/angular-cli/issues/9827#issuecomment-369578814

I think this library should not rely on some other process to supply the variable. So going forward you could use:

  • globalThis: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis
  • polyfill it: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis#search_for_the_global_across_environments

Workaround is adding (window as any).global = window; to polyfill.ts

DanielHabenicht avatar Apr 29 '22 09:04 DanielHabenicht

Is this about this line: https://github.com/Elringus/DotNetJS/blob/cad9a87854fd860203d25991a55bb3a68fa11229/DotNet/Packer/LibraryGenerator/LibraryTemplate.cs#L11 ?

It should only invoke in node environment, so global should be accessible.

elringus avatar Apr 29 '22 11:04 elringus

If it's that line, can you please test if the following will work with angular:

factory(module.exports, global || window); 

elringus avatar Apr 29 '22 13:04 elringus

It does not work, because global is not defined (so it produces the same error). You have to check if it exists before:

// Get global shim as in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis#search_for_the_global_across_environments
var getGlobal = function () {
  if (typeof self !== 'undefined') { return self; }
  if (typeof window !== 'undefined') { return window; }
  if (typeof global !== 'undefined') { return global; }
  throw new Error('unable to locate global object');
};

factory(module.exports, getGlobal()); 

DanielHabenicht avatar Apr 29 '22 13:04 DanielHabenicht

Wouldn't global || window return window in case global is not defined?

elringus avatar Apr 29 '22 14:04 elringus

Or should it be global ?? window.

elringus avatar Apr 29 '22 14:04 elringus

You can try it out in your own web browser, just open the Developer Tools and navigate to the console: image

It's kind of unsuspecting, because I would have said the same before I tried it out.

DanielHabenicht avatar Apr 29 '22 14:04 DanielHabenicht

Do you have any idea how we can make an automated test for this without involving angular?

elringus avatar Apr 29 '22 14:04 elringus

Execute the module in a browser context. This could be a way: https://www.sitepoint.com/using-es-modules/#:~:text=Safari%2C%20Chrome%2C%20Firefox%20and%20Edge,Here's%20what%20they%20look%20like.&text=Simply%20add%20type%3D%22module%22,executing%20each%20module%20only%20once.

Otherwise I think there is no way around implementing it inside an example project. Which would also be a way to test it.

DanielHabenicht avatar Apr 29 '22 14:04 DanielHabenicht

Wait, but it does already work in browser (there is a sample project). The line with global shouldn't be invoked in browser anyway.

elringus avatar Apr 29 '22 14:04 elringus

For webpack projects it is invoked because they are loaded as module (independent of later being executed in the browser). I think it would be the same for react or any other webframwork.

DanielHabenicht avatar Apr 29 '22 14:04 DanielHabenicht

I'm currently using the library in react, no issues here.

elringus avatar Apr 29 '22 14:04 elringus

Might be that react is already polyfilling global?

DanielHabenicht avatar Apr 29 '22 14:04 DanielHabenicht

I tried it out with stencil now, other error:

index-c37bab2d.js:2903 TypeError: Cannot set properties of undefined (setting 'dotnet')
    at app-home.entry.js:10:21
    at app-home.entry.js:11:3 undefined

Happen with or without the getGlobal fix.

import { Component, h } from '@stencil/core';
import * as dotnet from '../../../../Project/bin/dotnet';

@Component({
  tag: 'app-home',
  styleUrl: 'app-home.css',
  shadow: true,
})
export class AppHome {
  componentDidLoad() {
    dotnet.boot();
  }
  render() {
     ...
  }
}

DanielHabenicht avatar Apr 29 '22 14:04 DanielHabenicht

I think I was able to reproduce this in hello world sample with the browser module as you've suggested:

<!DOCTYPE html>

Open console (Ctrl+Shift+I) to see the output.


<script type="module">
    import * as dotnet from './Project/bin/dotnet.js'

    dotnet.HelloWorld.GetHostName = () => "Browser";

    window.onload = async function () {
        await dotnet.boot();
        const guestName = dotnet.HelloWorld.GetName();
        console.log(`Welcome, ${guestName}! Enjoy your global space.`);
    };

</script>

— but using the getGlobal() doesn't seem to help:

    var getGlobal = function () {
        if (typeof self !== 'undefined') { return self; }
        if (typeof window !== 'undefined') { return window; }
        if (typeof global !== 'undefined') { return global; }
        throw new Error('unable to locate global object');
    };
    if (typeof exports === 'object' && typeof exports.nodeName !== 'string')
        factory(module.exports, getGlobal());
    else factory(root.dotnet, root);

elringus avatar Apr 29 '22 14:04 elringus

The error seem to come from webpackUniversalModuleDefinition; could it be caused by the webpack autogenerated UMD wrapper?

elringus avatar Apr 29 '22 14:04 elringus

I think this is related: https://github.com/umdjs/umd/issues/127 and https://github.com/umdjs/umd/issues/134

a comment suggests to switch to esm, but I don't now if this is compatible with VSCode related targets

DanielHabenicht avatar Apr 29 '22 14:04 DanielHabenicht

Seems like the only way around is publishing two js files. One for ES modules and one as UMD.

DanielHabenicht avatar Apr 29 '22 15:04 DanielHabenicht

I've added browser-es sample for testing (not working right now): https://github.com/Elringus/DotNetJS/blob/main/Samples/HelloWorld/browser-es.html

Not sure what's the best approach to solve this (producing 2 libraries is not optimal, as it will require a lot of extra logic in C# packer). Guess the best workaround for now is polyfilling global on the consumer side. Will appreciate any suggestions.

elringus avatar Apr 29 '22 16:04 elringus

Might be that react is already polyfilling global?

More likely it's just I'm bundling the react app with webpack, which statically links all imports. If I got it right, this issue only matters when you try to directly import dotnet in browser as ES module. You can still use it with any framework or inside any app and then bundle it.

elringus avatar Apr 29 '22 16:04 elringus

Angular also uses Webpack under the hood. But alright you already changed the title of the issue to reflect on the ES module issue. Appreciate your investiagation.

DanielHabenicht avatar Apr 29 '22 16:04 DanielHabenicht

Well React is using kind of UMD only: https://github.com/facebook/react/issues/10021

DanielHabenicht avatar Apr 29 '22 17:04 DanielHabenicht

Maybe angular is bundled differently, without static linking the deps? I mean, if bundle already has all the imports resolved/linked internally at build time, it shouldn't matter which import mechanism each individual dependency uses when the bundle is imported at runtime, right?

elringus avatar Apr 29 '22 17:04 elringus

Well yes angular is already using esm modules. Would you be open to including the polyfill for getGlobal anyway?

DanielHabenicht avatar Apr 29 '22 17:04 DanielHabenicht

Would you be open to including the polyfill for getGlobal anyway?

If it'll solve the issue, sure. I've already tried doing that actually, but it didn't work, because we also need to share that global with dotnet/blazor internal stuff. But maybe I've missed something.

elringus avatar Apr 29 '22 17:04 elringus

If it'll solve the issue, sure. I've already tried doing that actually, but it didn't work, because we also need to share that global with dotnet/blazor internal stuff. But maybe I've missed something.

It is at least solving the issue for angular, as angular seems to use a different way for importing the module.

For importing the module via type"module" we would have to set this to window conditionally in dotnet.js:

!(function (e, t) {
    "object" == typeof exports && "object" == typeof module
        ? (module.exports = t())
        : "function" == typeof define && define.amd
        ? define([], t)
        : "object" == typeof exports
        ? (exports.dotnet = t())
        : (e.dotnet = t());
})(this, () => // <== Here
...

Then it is available via window.global.dotnet

DanielHabenicht avatar Apr 29 '22 18:04 DanielHabenicht

I've tried replacing this to window, but it returned Uncaught TypeError: dotnet.HelloWorld is undefined.

Regarding angular, do you mean it's not related with the browser-es sample? In this case we'd need a minimal repro (without angular) as a starting point. Ideally, an automated test or at least a sample for manual testing.

elringus avatar Apr 29 '22 20:04 elringus

I'm also not against dropping UMD target if there is a way to make it work with both CommonJS (require) and ES (import) in all the environments. Other UMD scenarios (global script import in browsers and AMD) are not that common nowadays, afaik.

elringus avatar Apr 29 '22 20:04 elringus

In my browser env, no global is defined, only globalThis @DanielHabenicht And then I don't think it has anything to do with the framework you're using (angular), because no browser will have a global var. In the browser (Worker or not) just call self, you will get var self: Window/WorkerGlobalScope & typeof globalThis

Aloento avatar Apr 30 '22 16:04 Aloento

I'm also not against dropping UMD target if there is a way to make it work with both CommonJS (require) and ES (import) in all the environments. Other UMD scenarios (global script import in browsers and AMD) are not that common nowadays, afaik.

If I were to do it, I would just write it all as an ES module and then polyfill it (like Babel) if needed.

Aloento avatar Apr 30 '22 16:04 Aloento

If I were to do it, I would just write it all as an ES module and then polyfill it (like Babel) if needed.

The thing is we have autogenerated mono wasm JS wrapper and then our own runtime module. Both are bundled with webpack on top of which C# packer emits UMD initialization template. As it currently stands, we have to first figure how to configure webpack to produce a module that can be imported as both ES and CommonJS in all the environments and then update the packer initialization template.

elringus avatar Apr 30 '22 16:04 elringus