The Wayback Machine - https://web.archive.org/web/20200908104452/https://github.com/scala-js/scala-js/issues/3743/
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

Avoid namespace imports in favor of named imports #3743

Open
vhiairrassary opened this issue Aug 17, 2019 · 6 comments
Open

Avoid namespace imports in favor of named imports #3743

vhiairrassary opened this issue Aug 17, 2019 · 6 comments
Labels

Comments

@vhiairrassary
Copy link

@vhiairrassary vhiairrassary commented Aug 17, 2019

Currently, for the following code:

object RawSemanticUi {
  @js.native
  @JSImport("semantic-ui-react", "Button")
  object Button extends js.Object
}

we were expecting to see something like:

import { Button } from "semantic-ui-react"

but the output is more like (scalajs 1.0.0-M8, fast and full opt.):

import * as $module from "semantic-ui-react";

// used later as:
const x = $module.Button;

The documentations mentions something similar.

We understand it may be considered as an optimization (and we have seen Is it a request for more optimizations? Technically not out of scope, but we have already enough ideas and not enough time for those.) but we wanted to know if it is the case here, or if there is a technical issue with named imports (as they may be used to improve tree shaking with webpack for example).

@sjrd
Copy link
Member

@sjrd sjrd commented Aug 18, 2019

There's no technical issue with named imports besides the fact that it's difficult to emit them in our compiler in a way that fits incremental linking.

We'll eventually do it, but we don't consider that a priority. A JavaScript-only pass can probably optimize such things on its own and do the tree shaking anyway.

@sjrd sjrd added the optimization label Aug 18, 2019
@vhiairrassary
Copy link
Author

@vhiairrassary vhiairrassary commented Aug 18, 2019

Thank you for your answer 👍

@gzm0 gzm0 changed the title Implementation details for named import of a member of a module Avoid namespace imports in favor of named imports Nov 28, 2019
@gzm0
Copy link
Contributor

@gzm0 gzm0 commented Mar 27, 2020

@sjrd Would it be acceptable to generate a and m env fields even for native classes? This would

  • use already existing namespacing mechanisms to select sub-fields of imports.
  • allow the required modules to be easily determined upon class construction (rather than relying on names like now).
  • make the load spec entirely private to the class (not required, but nice addition).

The downside is that we'd hide the access behind a function. The alternative would be to make a and m methods or fields depending on whether the class is native or not. But the semantic difference this implies probably violates the IR spec.

@sjrd
Copy link
Member

@sjrd sjrd commented Mar 27, 2020

I'd rather we used refined $i end fields so that they also include the top-level identifier. Then they can be shared across all classes that import that field, and we don't have an additional function.

@gzm0
Copy link
Contributor

@gzm0 gzm0 commented Apr 12, 2020

For folks following this issue. There is discussion on #4010 (comment) to use namespace imports for module splitting. If that get's accepted, we'd probably close this issue as wontfix (unless there is strong reason to do this for non-SJS modules, but not for SJS modules).

@gzm0
Copy link
Contributor

@gzm0 gzm0 commented Apr 14, 2020

Relevant discussion on gitter:

@gzm0

Yes, that is fair. But by fixing #3743, don't we acknowledge that "we need precise non-namespace imports"? At least the current reasoning in #3743 applies to any kind of import.

@sjrd

Not necessarily. Because internally, we already have dead code elimination. We eliminate a log of things that don't make it into modules at all. External dependencies don't have that luxury. For them, avoiding namespace imports can be more important.

So #4010 is less related to this than I initially thought.

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.