Skip to content

ES6 modules don't work in DDC, remove library-root, module-root #32272

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

Open
jmesserly opened this issue Feb 21, 2018 · 6 comments
Open

ES6 modules don't work in DDC, remove library-root, module-root #32272

jmesserly opened this issue Feb 21, 2018 · 6 comments
Assignees
Labels
area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. dev-compiler-ux type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) web-dev-compiler

Comments

@jmesserly
Copy link

There are two problems:

DDC imports with unqualified names like "foo" instead of relative paths like "./foo". While this is supported by existing JS module systems, it is not supported by built-in ES6 module loaders. We also need to remove module-root because it's not the right UX. Instead we should allow the user to provide the JS import path along with a summary for that module, with some sort of default behavior (probably flat modules: "./module_name"). We'll also need a way to provide the SDK import path.

The other problem is with library names. They are exports of temporary identifiers, which can be renamed, breaking the module's ABI. It works in existing JS module systems, because DDC desugars ES6 export into a renamed local and a non-renamed export. We should be able to fix this by using export as. I think import already has a way to handle TemporaryId names and generate as when needed.

(Original proposal/discussion here: #27262)

@jmesserly jmesserly added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) web-dev-compiler labels Feb 21, 2018
@jmesserly jmesserly self-assigned this Feb 21, 2018
@nex3
Copy link
Member

nex3 commented Feb 21, 2018

Synthesizing/revising my proposal from the original thread:

I propose that the --summary flag be extended to allow the form --summary=path,import1 where path is the path (or ideally URL; see #27266) to the summary and import is the URL to use to import it. The import URL would default to the relative URL to the summary file from the output file, without the summary extension2 and with a ./ prefix3. So for example:

  • dartdevc app.dart -o app.js --summary build/lib.sum would generate:

    import { /* ... */ } from './lib';
  • dartdevc app.dart -o app.js --summary build/lib.sum,../shared/lib would generate:

    import { /* ... */ } from '../shared/lib';
  • dartdevc app.dart -o app.js --summary build/lib.sum,http://localhost:1234/lib would generate:

    import { /* ... */ } from 'http://localhost:1234/lib';

I also propose an --sdk-import flag that defines the URL from which the SDK module is imported. This would default to dart_sdk, as it does today.


  1. I'm using , as a separator here rather than my original proposal of :, both to match the existing --url-mapping flag and because , is much less likely to conflict with URL syntax.

  2. My original proposal just used the basename of the file because it was derived from a proposal about the old custom module loader, but I think in an ES6-first world relative URLs make more sense.

  3. I don't think this prefix is meaningful for browsers, but for Node.js it's used to signal a relative import (as opposed to a load-path import).

@jmesserly
Copy link
Author

import { /* ... */ } from 'lib';

did you mean "./lib" here, because of the rule "with a ./ prefix"

  1. I don't think this prefix is meaningful for browsers, but for Node.js it's used to signal a relative import (as opposed to a load-path import).

Yeah, I need to double check the status of this in browsers. At one point it seemed like explicit relative path syntax was required. In any case, I agree with your point in 3--since browsers only support relative paths, it's nice to use that across all of our module formats for consistency.

@nex3
Copy link
Member

nex3 commented Feb 21, 2018

did you mean "./lib" here, because of the rule "with a ./ prefix"

Yep, fixed, good catch.

@jmesserly
Copy link
Author

https://dart-review.googlesource.com/c/sdk/+/42892 addresses the library name export bug. Need another CL for the module-root issue.

@jmesserly jmesserly mentioned this issue Feb 22, 2018
16 tasks
dart-bot pushed a commit that referenced this issue Mar 7, 2018
We use renamable variables for Dart libraries, this ensures we still
export it with the correct name.

Change-Id: I96dc161e33d265c0ffbd07f8d642629504dffe62
Reviewed-on: https://dart-review.googlesource.com/42892
Commit-Queue: Jenny Messerly <[email protected]>
Reviewed-by: Vijay Menon <[email protected]>
@jmesserly
Copy link
Author

the export name issue is fixed in 1b9b345 ... library-root CL is under development, and module-root still needs to be looked at.

@jmesserly jmesserly changed the title ES6 modules don't work in DDC ES6 modules don't work in DDC, remove library-root, module-root May 4, 2018
@jmesserly
Copy link
Author

jmesserly commented Jul 13, 2018

progress on deprecating module-root: https://dart-review.googlesource.com/c/sdk/+/64823

It appears the option of passing the import name was already implemented (though it needed some refactoring). For example -s summary_path=js_import_path. I added an option to specify the output module name from the original proposal (used by some module formats like AMD and DDC's legacy format, ES6 and CommonJS do not need it).

Next up is bringing these new options over to dartdevk (it has never supported module-root or library-root, but it also does not support the correct way of providing import name via --summary or the new --module-name option).

dart-bot pushed a commit that referenced this issue Jul 26, 2018
Adds an option for specifying the output JS module name if that is
needed (only applies for some module formats).

Also removes repl-compile (it's set via API, not the command line).

Refactors dartdevk options to match dartdevc so we can migrate more
easily. Moves shared code into a shared location and removes copied
code.

Change-Id: I966343ecbbc962f5d0f14ea7e65d78660159f420
Reviewed-on: https://dart-review.googlesource.com/64823
Commit-Queue: Jenny Messerly <[email protected]>
Reviewed-by: Bob Nystrom <[email protected]>
@vsmenon vsmenon added the area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. label Jul 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. dev-compiler-ux type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) web-dev-compiler
Projects
None yet
Development

No branches or pull requests

3 participants