Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bug] deno bundle generate broken code #4031

Closed
saostad opened this issue Feb 18, 2020 · 13 comments · Fixed by #4042
Closed

[bug] deno bundle generate broken code #4031

saostad opened this issue Feb 18, 2020 · 13 comments · Fixed by #4042

Comments

@saostad
Copy link

saostad commented Feb 18, 2020

deno 0.33.0
v8 8.1.108
typescript 3.7.2

I tried to bundle a file with content code:
console.log('Hello from Deno!');

to bundle: deno bundle app.ts bundle.js

it generated :

// Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.

// This is a specialised implementation of a System module loader.

// eslint-disable-next-line @typescript-eslint/no-unused-vars
let System;
let __inst;

(() => {
  const mMap = new Map();
  System = {
    register(id, deps, f) {
      mMap.set(id, {
        id,
        deps,
        f,
        exp: {}
      });
    }
  };

  const gC = (data, main) => {
    const { id } = data;
    return {
      id,
      import: async id => mMap.get(id)?.exp,
      meta: { url: id, main }
    };
  };

  const gE = data => {
    const { exp } = data;
    return (id, value) => {
      const values = typeof id === "string" ? { [id]: value } : id;
      for (const [id, value] of Object.entries(values)) {
        Object.defineProperty(exp, id, {
          value,
          writable: true,
          enumerable: true
        });
      }
    };
  };

  const iQ = [];

  const enq = ids => {
    for (const id of ids) {
      if (!iQ.includes(id)) {
        const { deps } = mMap.get(id);
        iQ.push(id);
        enq(deps);
      }
    }
  };

  const dr = async main => {
    const rQ = [];
    let id;
    while ((id = iQ.pop())) {
      const m = mMap.get(id);
      const { f } = m;
      if (!f) {
        return;
      }
      rQ.push([m.deps, f(gE(m), gC(m, id === main))]);
      m.f = undefined;
    }
    let r;
    while ((r = rQ.shift())) {
      const [deps, { execute, setters }] = r;
      for (let i = 0; i < deps.length; i++) setters[i](mMap.get(deps[i])?.exp);
      const e = execute();
      if (e) await e;
    }
  };

  __inst = async id => {
    System = undefined;
    __inst = undefined;
    enq([id]);
    await dr(id);
    return mMap.get(id)?.exp;
  };
})();

console.log('Hello from Deno!');

await __inst("file:///C:/Users/SOstad/Projects/tests/deno01/src/app");

after I run it with deno run bundle.js here is the error:

error: Uncaught TypeError: Cannot destructure property 'deps' of 'mMap.get(...)' as it is 
undefined.

also Docs for bundle is outdated:

Bundling

deno bundle [URL] will output a single JavaScript file, using AMD, which includes all dependencies of the specified input.

> deno bundle https://deno.land/std/examples/colors.ts
Bundling "colors.bundle.js"
Emitting bundle to "colors.bundle.js"
9.2 kB emitted.

To run then bundle in Deno use

deno https://deno.land/std/bundle/run.ts colors.bundle.js

Bundles can also be loaded in the web browser with the assistance of RequireJS. Suppose we have a bundle called website.bundle.js, then the following HTML should be able to load it:

<script src="https://cdnjs.cloudflare.com/ajax/libs/require.js/2.3.6/require.min.js"></script>
<script src="website.bundle.js"></script>
<script>
  requirejs(["website"], website => website.main());
</script>

Here we assume there's an exported function main() from website.ts.

// website.ts
export main() {
  console.log("hello from the web browser");
}
@kitsonk
Copy link
Contributor

kitsonk commented Feb 18, 2020

Your input file isn't being detected as a module... it is a "limitation" of TypeScript (or ECMAScript depending on how you see it) (see: microsoft/TypeScript#18232).

Try this as your source file:

console.log('Hello from Deno!');
export {};

It should resolve the issue.

As far as the documents, I will raise a PR to update them.

@kitsonk
Copy link
Contributor

kitsonk commented Feb 19, 2020

Actually, looking, the documents are accurate and don't represent the version you pasted in here. 😕

@saostad
Copy link
Author

saostad commented Feb 19, 2020

I was reading the readme from here
it looks like it's old!
also even with code

console.log('Hello from Deno!');
export const myVar = 12 ;

I am getting same error:

deno ./src/bundle.js
error: Uncaught TypeError: Cannot destructure property 'deps' of 'mMap.get(...)' as it is 
undefined.
► file:///C:/Users/SOstad/Projects/tests/deno01/src/bundle.js:50:17

50         const { deps } = mMap.get(id);
                   ^

    at enq (file:///C:/Users/SOstad/Projects/tests/deno01/src/bundle.js:50:17)
    at __inst (file:///C:/Users/SOstad/Projects/tests/deno01/src/bundle.js:81:5)
    at file:///C:/Users/SOstad/Projects/tests/deno01/src/bundle.js:100:21

Generated bundle code:

// Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.

// This is a specialised implementation of a System module loader.

// eslint-disable-next-line @typescript-eslint/no-unused-vars
let System;
let __inst;

(() => {
  const mMap = new Map();
  System = {
    register(id, deps, f) {
      mMap.set(id, {
        id,
        deps,
        f,
        exp: {}
      });
    }
  };

  const gC = (data, main) => {
    const { id } = data;
    return {
      id,
      import: async id => mMap.get(id)?.exp,
      meta: { url: id, main }
    };
  };

  const gE = data => {
    const { exp } = data;
    return (id, value) => {
      const values = typeof id === "string" ? { [id]: value } : id;
      for (const [id, value] of Object.entries(values)) {
        Object.defineProperty(exp, id, {
          value,
          writable: true,
          enumerable: true
        });
      }
    };
  };

  const iQ = [];

  const enq = ids => {
    for (const id of ids) {
      if (!iQ.includes(id)) {
        const { deps } = mMap.get(id);
        iQ.push(id);
        enq(deps);
      }
    }
  };

  const dr = async main => {
    const rQ = [];
    let id;
    while ((id = iQ.pop())) {
      const m = mMap.get(id);
      const { f } = m;
      if (!f) {
        return;
      }
      rQ.push([m.deps, f(gE(m), gC(m, id === main))]);
      m.f = undefined;
    }
    let r;
    while ((r = rQ.shift())) {
      const [deps, { execute, setters }] = r;
      for (let i = 0; i < deps.length; i++) setters[i](mMap.get(deps[i])?.exp);
      const e = execute();
      if (e) await e;
    }
  };

  __inst = async id => {
    System = undefined;
    __inst = undefined;
    enq([id]);
    await dr(id);
    return mMap.get(id)?.exp;
  };
})();

System.register("app", [], function (exports_1, context_1) {
    "use strict";
    var myVar;
    var __moduleName = context_1 && context_1.id;
    return {
        setters: [],
        execute: function () {
            console.log('Hello from Deno!');
            exports_1("myVar", myVar = 12);
        }
    };
});

const __exp = await __inst("file:///C:/Users/SOstad/Projects/tests/deno01/src/app");
export const myVar = __exp["myVar"];

@IllusionPerdu
Copy link

It's the line
const __exp = await __inst("file:///C:/Users/SOstad/Projects/tests/deno01/src/app");
it's sould be
const __exp = await __inst("app");

or

the line
System.register("app", [], function (exports_1, context_1) {
should be
System.register("file:///C:/Users/SOstad/Projects/tests/deno01/src/app", [], function (exports_1, context_1) {

@IllusionPerdu
Copy link

For the case #4031 (comment) the bundler shoud don't add await __inst("file:///C:/Users/SOstad/Projects/tests/deno01/src/app");

I think it's two different bug!

@kitsonk
Copy link
Contributor

kitsonk commented Feb 19, 2020

@bartlomieju we should remove the GitHub site on the repo (https://denoland.github.io/deno/) as it contains an old version of the website 😢.

@IllusionPerdu you are right, the analysis we do when there is a single module is an additional problem, we get it wrong, we are "guessing" and we get it wrong. That needs to be fixed. Still before the export it was a different problem, but no, the await is intentional and needs to be there, as the whole process is asynchronous and before we do any exports, it needs to be resolved.

@ry
Copy link
Member

ry commented Feb 19, 2020

@kitsonk Thanks for the notice - I've updated the gh-pages index.html

kitsonk added a commit to kitsonk/deno that referenced this issue Feb 20, 2020

Verified

This commit was signed with the committer’s verified signature.
oddgrd Oddbjørn Grødem
Fixes denoland#4031

When a bundle contains a single module, we were incorrectly determining
the module name, resulting in a non-functional bundle.  This PR corrects
that determination.
@ry ry closed this as completed in #4042 Feb 20, 2020
ry pushed a commit that referenced this issue Feb 20, 2020

Verified

This commit was signed with the committer’s verified signature.
oddgrd Oddbjørn Grødem
Fixes #4031

When a bundle contains a single module, we were incorrectly determining
the module name, resulting in a non-functional bundle.  This PR corrects
that determination.
@agusbena
Copy link

agusbena commented Feb 24, 2020

Hi 80.0.3987.116

For the case #4031 (comment) the bundler shoud don't add await __inst("file:///C:/Users/SOstad/Projects/tests/deno01/src/app");

I think it's two different bug!

I am using the latest Chrome (80.0.3987.116) and the top level await is still not supported in this version, So the bundle fails to load:

Uncaught SyntaxError: Unexpected reserved word
... for the :
const __exp = await __inst("MyModule");

Top level await is still in stage 3 it seems:
https://github.com/tc39/proposal-top-level-await

@kitsonk
Copy link
Contributor

kitsonk commented Feb 24, 2020

My personal opinion is that we don't hold up support top level await in bundles. We have been supporting it in Deno ever since it was implemented in V8 (and wanted to support it earlier).

The only solution is that if the main module has no exports, we could drop the await, but it if does, we need to have the await there.

@ry thoughts?

@ry
Copy link
Member

ry commented Feb 24, 2020

@kitsonk I agree. I think if people are deploying the bundle to web they ought to just not use TLA...

@kitsonk
Copy link
Contributor

kitsonk commented Feb 24, 2020

@ry, well the problem is they don't have a choice now, because the bundle loader works in an async way in order to accomodate bundling modules that require TLA, which means that we have to instantiate the loader using TLA. So until browsers support TLA, it means Deno bundles would not work in browsers.

@agusbena
Copy link

agusbena commented Feb 24, 2020

@kitsonk
there could be a parametter for:
deno bundle --no-tla
...or something, how hard it might be to implement?

When i showed deno at my office one of the features we understood will be really useful for us was reuse of server module in the browser by just deno bundle them.
Thanks you for listen

@kitsonk
Copy link
Contributor

kitsonk commented Feb 24, 2020

While it might be complicated, I think I can detect when a bundle contains modules that are using TLA, and pass in something that will change the instantiation from async to sync. Let me see if I can do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants