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

Fix #215 - Streamline rollup build process and add browser bundle to npm package #216

Merged
merged 9 commits into from Jan 27, 2021

Conversation

acmiyaguchi
Copy link
Contributor

@acmiyaguchi acmiyaguchi commented Jan 16, 2021

This fixes #216. This adds the browser assets into the npm package so it can be used by web application bundlers. This also simplifies the build process in a few ways.

  • There is now a single rollup configuration that builds both the node and browser bundles
  • Add a prepare script for installing directly from a git branch
  • Version information is pulled from the package.json and injected using a rollup plugin. In the Makefile, npm info biwascheme version is used as the source of truth for updating the website.
  • Git information can be directly added to the bundle, instead relying on the Makefile (which makes the build cross platform).

I put together a small test application that is deployed to this site. I exported the BiwaScheme module so I didn't have to rely on the module attached to the window. It might be worthwhile to expose the platform agnostic modules to npm directly (tree-shaking and all) so web applications can write their own ports without relying on jquery.

@acmiyaguchi acmiyaguchi marked this pull request as ready for review January 16, 2021 23:23
@acmiyaguchi
Copy link
Contributor Author

I followed a troubleshooting guide for rollup in order to deal with the following errors.

> Use of eval is strongly discouraged, as it poses security risks and may cause issues with minification        
5367:   //
5368:   define_libfunc("js-eval", 1, 1, function (ar) {
5369:     return eval(ar[0]);
                 ^
5370:   });
5371:   define_libfunc("js-ref", 2, 2, function (ar) {

> Use of eval is strongly discouraged, as it poses security risks and may cause issues with minification        
5423:     // TODO: convert lambdas by js-closure
5424:     if (isSymbol(receiver)) {
5425:       receiver = eval(receiver.name); //XXX: is this ok?
                       ^
5426:     }

> Use of eval is strongly discouraged, as it poses security risks and may cause issues with minification        
5510:
5511:     var ctor = ar.shift();
5512:     if (isString(ctor)) ctor = eval(ctor);
                                     ^
5513:
5514:     if (ar.length == 0) {

@yhara
Copy link
Member

yhara commented Jan 21, 2021

I followed a troubleshooting guide for rollup in order to deal with the following errors.

Oh I didn't know there is a way to avoid this warning. Great!

@yhara yhara merged commit 75adfd6 into biwascheme:master Jan 27, 2021
@yhara
Copy link
Member

yhara commented Jan 28, 2021

@acmiyaguchi Thank you for your pull request! The build process is much cleaner now.

It might be worthwhile to expose the platform agnostic modules to npm directly (tree-shaking and all) so web applications can write their own ports without relying on jquery.

I didn't understand well. What do you mean by "directly" -- adding src/*.js to the "files" section of package.json?
Currently biwascheme.js bundles src/deps/jquery-3.5.1-esm.js in it but it is nice if users can use their own version of jQuery, or choose not using jQuery at all.

@acmiyaguchi
Copy link
Contributor Author

I didn't understand well. What do you mean by "directly" -- adding src/*.js to the "files" section of package.json?
Currently biwascheme.js bundles src/deps/jquery-3.5.1-esm.js in it but it is nice if users can use their own version of jQuery, or choose not using jQuery at all.

I'm not entirely clear about this myself, but I think this would require refactoring in order to exclude jQuery/underscore. One idea is to have an interface with optional node/browser implementations might help with alternative use cases:

import BiwaScheme from "biwascheme";
import { Console, Port, Dumper } from "biwascheme/browser";

// or a callback functions instead of pluggable objects to be overridden
let biwascheme = new BiwaScheme({Console: Console, Port: Port, Dumper: Dumper});

If you don't need the default implementation, then you could write your own without the extra dependencies. If you write an application using rollup/webpack, then it should be able to exclude the default modules since they won't be use. CodeMirror 6 has an modular architecture that might be a useful source of ideas.

On another note, maybe it would make sense to make JQuery/lodash a peer dependency so authors can include their own versions? I'm not sure how excluding the dependencies works without a bundler -- possibly two versions of the distributed files e.g. biwascheme.min.js and biwascheme-no-jquery.min.js.

@yhara yhara added this to the v0.7.2 milestone Mar 11, 2021
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.

None yet

2 participants