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

Compile TS to native JS modules #2858

Closed
phaux opened this issue Sep 24, 2017 · 32 comments
Closed

Compile TS to native JS modules #2858

phaux opened this issue Sep 24, 2017 · 32 comments
Labels
feature PRs and issues for features

Comments

@phaux
Copy link

phaux commented Sep 24, 2017

RxJS version: 5.4.3

Code to reproduce:

<script type="module">
import * as Rx from './node_modules/rxjs/Rx.js'
</script>

Expected behavior: There should be a way to load RxJS via native JS module

Actual behavior: Can't access Rx when using JS modules (error: require is not defined)

@kwonoj
Copy link
Member

kwonoj commented Sep 24, 2017

This one requires build target for ES2015 modules, which we started include in #2804 . I think there'll be still some churns to actually able to use it but please give it a shot with 5.5 b.

@kwonoj kwonoj added the feature PRs and issues for features label Oct 6, 2017
@phaux
Copy link
Author

phaux commented Oct 23, 2017

I tried again with RxJS 5.5.0 and the only thing that is missing are js extensions in import statements. Without them I get 404 errors.

@phaux
Copy link
Author

phaux commented Oct 23, 2017

I configured the server to rewrite urls to include the extension and it works! Another problem that remains is that typescript can't find typings for the imports.

@kaste
Copy link

kaste commented Oct 26, 2017

@kwonoj I tried the newer esm2015 build. But for native browser support you can't omit the file extension '.js' on import statements. AFAIK this won't be added as compile step to tsc. So there is the option to post-process the built files, or to add the extension everywhere in the source ts files. This looks ugly or unfamiliar, but I think people are doing this already.

@phaux
Copy link
Author

phaux commented Oct 26, 2017

From my experience the typings files should be emitted together with JS in the same directory

@kwonoj
Copy link
Member

kwonoj commented Oct 26, 2017

I am not using native modules at this moment so bit unclear things to me, please share details if you don't mind.

  1. about extension
    : is this meaning you can't import esm as native module? or it's another enhancement to be added?
  2. typings
    : what does this exactly mean with native module import environments?

@phaux
Copy link
Author

phaux commented Oct 26, 2017

about extension

Take a look at the file @reactivex/rxjs/dist/esm2015/Observable.js. All the imports there should have .js extensions. Otherwise importing Observable into your app will fail with 404 error (e.g. ./util/root should be ./util/root.js). I worked around that by setting up URL rewriting in apache/nginx.

typings

Example code:

import {Observable} from '../node_modules/@reactivex/rxjs/dist/esm2015/Observable.js'

The detected type of Observable is any. It should work if the declarations from /dist/typings were also copied to /dist/esm2015.

It seems that tsc will only look at "typings" in package.json when importing a module using CJS/Node syntax (import "@reactivex/rxjs" without mentioning node_modules or subpath)

@kwonoj
Copy link
Member

kwonoj commented Oct 26, 2017

/cc @jasonaden for visibility. Seems this requires postprocessing script.

@phaux
Copy link
Author

phaux commented Oct 26, 2017

I think it would make more sense if such script was implemented as part of build process in typescript itself after all 🤔

@kwonoj
Copy link
Member

kwonoj commented Oct 26, 2017

Maybe worth to ask tsc team as issue? (Or already there is?)

@kaste
Copy link

kaste commented Oct 27, 2017

To quote myself

AFAIK this won't be added as compile step to tsc.

I actually went to tsc first b/c I thought that's their fault, but from reading the issues I got the feeling they don't want to do that. BUT ... from their replies, I also got the feeling that they didn't quite get what the users wanted and thought 'user fault' too early. An issue raised by RxJS would be treated differently.

I don't think that they can get away with compiling to esm2015 but not supporting the browser import loader. I mean the browser loader is by far more important than the node module resolution algo and rollup and the rest of them together.

And AFAIK the browser agreed on that they only accept fully qualified names (in the end: URLs) as module names.

@phaux
Copy link
Author

phaux commented Oct 27, 2017

Let's sum up all the ES module quirks again:

  1. Imports must include .js extensions, because they are basically relative urls that the browser will use to download scripts.
  2. Imports of other libraries must use relative path (../node_modules/some-lib/dist/lib.js instead of just some-lib)
  3. Typings (.d.ts files) should be in the same directory that the js files are. "typings" property of package.json is used only when importing a library using node syntax (using only the package name as the path, so it doesn't work with ES modules)

(3) is actually the default behavior of TS. (1) and (2) would need a post processing script if TS wont implement it.

Post processing script pseudo code:

for (every Import statement) {

    if (Import points to other package) {
        
        path = find_relative_path_to('node_modules') + path
        path += read('package.json')['main'] // read main property from module's package.json

    }
    
    if (path points to directory) { path += '/index.js' }
    else { path += '.js' }

}

@kaste
Copy link

kaste commented Dec 20, 2017

Friendly ping.

Any progress on this? What direction will Rx go?

/Cc @benlesh you asked me for a link to this issue.

@kwonoj
Copy link
Member

kwonoj commented Dec 20, 2017

@kaste We do not have dedicated plan / progress for this at this moment, as we haven't deeply explored which way we'll go.

@kaste
Copy link

kaste commented Dec 20, 2017

🔮

@matthewp
Copy link

I can take this one. I wrote a babel plugin that does exactly this. Since you do not currently use Babel I imagine that you likely don't want to add it. No worries! I'm happy to write a cli tool that wraps this, so that's all that will need to be added as a devDependency.

If this plan sounds fine to the maintainers I am happy to submit a PR.

@kwonoj
Copy link
Member

kwonoj commented Jan 12, 2018

I am actually not very opposed to have babel as dep, if it doesn't hurt bulid time too much. In my understanding this would be required only some of pkg (like ESM) only - is that correct? /cc @benlesh for babel side options. + if we're going to use babel, we need to ensure build output shouldn't be mutated other than import path adjustment.

@matthewp
Copy link

@kwonoj It would only run that single babel plugin that appends the .js, and probably only for the esm2015 build.

I understand if you don't want all of the ceremony of adding babel though, so I'm happy to tuck this away into a cli tool to keep things tidy here if that's what you prefer.

@benlesh
Copy link
Member

benlesh commented Jan 12, 2018

It seems like we should be able to do this with TypeScript or TS transformations. I'll ask around

@kwonoj
Copy link
Member

kwonoj commented Jan 12, 2018

@benlesh this seems current discussion proposal: microsoft/TypeScript#16577

@matthewp
Copy link

@benlesh @kwonoj Yeah, I looked there first, seems like there isn't a good solution on the TS side at this time, because they don't actually know that the module identifier points at a file (vs. a folder for example).

@kwonoj
Copy link
Member

kwonoj commented Jan 12, 2018

One arbitrary question: How widely native module import is being used currently? is this something we should tackle immediately, or either something we can wait until TS have conclusions to support it? My initial guess is this probably only runs on latest browser, have limited usage (but I don't write browser target quite lot, so could be wrong)

@phaux
Copy link
Author

phaux commented Jan 15, 2018

@kwonoj I personally need native modules for experimenting with Polymer 3.0 so it's not that important.

@kaste
Copy link

kaste commented Jan 15, 2018

@kwonoj Do you expect a guessing answer here or one that is based on facts? 😺

@Buslowicz
Copy link

@kwonoj browser support for native modules is pretty good and once FF 60 is out, it will be natively supported everywhere. According to caniuse.com, the current browser support is 68% of all browsers and versions in use, and once FF gets it it will be around 72%, which is pretty major and I am sure it will jump up closer to 80% very soon, meaning a lot of companies will jump over to it.
I am aware TS could do a lot of the job here, but it would really be handy if the esm2015 output would follow the valid ESM specs sooner ;).

@kwonoj
Copy link
Member

kwonoj commented Sep 25, 2018

v6 exports esm / esm2015 altogether, while mjs is not there but it's not stable so out of scope for this issue I believe.

@kwonoj kwonoj closed this as completed Sep 25, 2018
@matthewp
Copy link

I don't see a module bundle in v6. Where is it? I see the umd bundle is in bundles/ but I don't see a modules bundle any where.

@kwonoj
Copy link
Member

kwonoj commented Sep 25, 2018

@matthewp in pkg, it have _esm2015 and _esm5 and package.json speficies each in module / es2015 field: https://unpkg.com/rxjs@6.3.2/package.json , https://unpkg.com/rxjs@6.3.2/_esm2015/ https://unpkg.com/rxjs@6.3.2/_esm5/

@kwonoj
Copy link
Member

kwonoj commented Sep 25, 2018

I don't see a module bundle in v6.

Not sure if I understood this correctly, do you expect some other formats then esm output itself?

@matthewp
Copy link

Ok, I didn't see those folders. I see them now. Looking at them, they still omit the .js extension, so they are not loadable in a browser. I think this issue should be reopened.

@kwonoj
Copy link
Member

kwonoj commented Sep 25, 2018

@matthewp uh, I see. Actually this issue itself is bit more broad (it was moment we have esm5 only) #3858 is right scoped one, but locked due to old issues. Probably we just need file new one instead.

@matthewp
Copy link

I think this is the right scoped one, right? The OP is showing an example of use in the browser.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature PRs and issues for features
Projects
None yet
Development

No branches or pull requests

6 participants