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 to UMD and add AMD main.js #20

Closed
wants to merge 4 commits into from

Conversation

kitsonk
Copy link

@kitsonk kitsonk commented Jul 15, 2016

This PR resolves ReactiveX/rxjs#1664.

  • Adds Babel Plugin babel-plugin-transform-es2015-modules-umd to compile the modules to a format that can be loaded by both CommonJS and AMD modules.
  • Adjusts the .babelrc to utilise the plug-in
  • Adds a main.js which is an AMD module that replicates the functionality of index.js
  • Modifies the package.json to add both Babel plugin as a development dependency as well add the main.js to the files
    • Note that files in package.json refers to ponyfill.js which appears to not be part of the package, though lib/ponyfill.js appears to be present and referred to as well. I did not address this.
  • Modified the readme.md to reflect usage.
    • Note, since the lib/index.js has a ES2015 default export, the example shown in the readme was incorrect. I have corrected this.

I tested this within a TypeScript project that uses the dojo/loader which properly loaded the package, that was located in the node_modules of the project with the following AMD loader config:

require.config({
    packages: [
        { name: 'symbol-observable', location: 'node_modules/symbol-observable' }
    ]
});

The loader properly loaded the symbol-obersevable/main.js which in turn loaded the lib/index.js and then the lib/ponyfill.js.

@juristr
Copy link

juristr commented Aug 3, 2016

@Blesh Any chance we can get this fix into rxjs 5? I know using AMD (requirejs) is stone-age tech and I completely agree. I'm doing Angular 2 and modern JS, but I have an older project where we couldn't yet afford to upgrade to ES6/TypeScript but I still wouldn't want to miss the power of RxJS.

Plz 🙏 ☺️

@juristr
Copy link

juristr commented Aug 3, 2016

@kitsonk How did u think about including this library with RxJS and requirejs/amd standard?

I just manually imported your changes on my node_modules/symbols-observable folder and configured requirejs like

require.config({
  paths: {
     ...
     rxjs: '../../node_modules/@reactivex/rxjs/dist/amd',
    'symbol-observable': '../../node_modules/symbol-observable/main'
  }
})

That way the symbol-observable import in the RxJS AMD Subject file works properly, however the ./lib/index import in your fix here doesn't resolve. Can you share how you configured RequireJS?

Couldn't we just create a UMD/AMD bundle of this lib so that AMD ppl could simply import that one, just in the same fashion as RxJS does it? thx for any hint

@benlesh
Copy link
Owner

benlesh commented Aug 3, 2016

I was out on medical when this landed so I missed it at first so for the delay. I'll review this as soon as I can

@juristr
Copy link

juristr commented Aug 3, 2016

@Blesh 😍 Let me know if u need anything. As mentioned, the best IMHO would be to create a AMD bundle, basically one file containing everything. That way one can configure his requirejs paths and no changes would be needed on other libs.

I'm currently testing something like this on my local machine, just manually creating an AMD bundle. I have RxJS 5 already running with AMD (with the exception of this issue here). That way, once we've resolved this, I can test the whole setup and we could also close ReactiveX/rxjs#1664 on the main RxJS repo 😃

@juristr
Copy link

juristr commented Aug 3, 2016

@Blesh @kitsonk Just FYI. I just used this within my AMD based RxJS setup and it worked. Here's the bundle I just manually assembled, so don't look at the code 😜 https://github.com/juristr/symbol-observable-amd/tree/master/bundle

@benlesh
Copy link
Owner

benlesh commented Aug 9, 2016

Okay, I'm looking at getting this merged today, however there is a slight issue that was introduced by the need to make sure we were outputting valid ES3 (from #22)... After rebasing this PR locally to test it, it looks like what's being output now is using exports.default instead of exports["default"].

@juristr
Copy link

juristr commented Aug 9, 2016

Hm...well, honestly, rather than merging this PR, I'd suggest adding a task which bundles all files into a single AMD compatible bundle, like I linked above in my comment. I've tested that setup with my project which extensively uses RequireJS and AMD and it works perfectly, with the latest RxJS 5 beta.10 release (without any change on that side).

@kitsonk
Copy link
Author

kitsonk commented Aug 9, 2016

I can update the PR.

@juristr that precludes other builders from working effectively and further bundling and optimisation might be impacted as well. It is great that it works for you, but we should produce something that works for everyone.

@juristr
Copy link

juristr commented Aug 9, 2016

@kitsonk Sure, I'm willing to collaborate on this side, obviously it shouldn't be a solution for a single use case. 👍

Had some issues in getting your PR to work with my setup. I'll retry tomorrow and then report them here.

@benlesh
Copy link
Owner

benlesh commented Sep 16, 2016

Going through my OSS debt...

I think that we should be publishing a UMD file for this. @kitsonk where do you stand on conflict resolution for this PR.

@kitsonk
Copy link
Author

kitsonk commented Oct 31, 2016

Ok, I finally got time to refresh this. The problem now is that there appear to be incompatibilities with the babel-plugin-transform-es2015-modules-umd plugin which causes it in certain situations to emit code that is incompatible with ES3, which I have opened an issue at babel/babel#4799. So until then, this PR will produce modules that won't run under ES3.

@benlesh
Copy link
Owner

benlesh commented Dec 9, 2016

@kitsonk FYI: I'm digging into this today (I know it's been a LONG time)... it seems that Babel's transform for this is producing invalid ES3. I haven't quite figured out a fix, but I'm working on it.

@benlesh
Copy link
Owner

benlesh commented Dec 9, 2016

@kitsonk There is a [fix defined here] that works for this PR... however, I'm still not sure this PR does what we want. It seems the outcome here is two separate UMD files, where it's probably more likely that we want a single UMD bundled output.

@benlesh
Copy link
Owner

benlesh commented Aug 11, 2017

Closing this out of inactivity. I haven't seen an issue that requires this come up in production, nor has there been any additional outside interest.

@benlesh benlesh closed this Aug 11, 2017
@developingdenny
Copy link

developingdenny commented Sep 20, 2017

The README currently cites an option to build AMD loadable version with build_amd, but this option does not appear in the package.json "scripts" key. Are there plans to add it?

If not, what is the best way to get rxjs into AMD loadable modules for use in existing AMD projects? Using TypeScript's "module": "amd" configuration to transpile the rxjs/src?

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 this pull request may close these issues.

Issues with symbol-observable and AMD
4 participants