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

Babel transform everything for older browsers #24

Closed
wants to merge 2 commits into from

Conversation

dawsbot
Copy link

@dawsbot dawsbot commented Apr 13, 2020

Closes #23

I tested this locally in the Everipedia web application and successfully created https://bloks.io/transaction/e85fc2537f739d9912107071f2e48cc110e2b43ba5ede257c53ed5d5a5cd5e8f using Scatter.

If merging this is too risky for y'all just wait a week, we're going to publish this and use it in production immediately to see if this mitigates the issue we're seeing.

File Sizes (NO CHANGE) 🙌

Obtained via ls -lh | awk '{print $9, $5}' | grep "\.js"

Before:

dfuse-client.es5.js 95K
dfuse-client.es5.js.map 176K
dfuse-client.umd.js 48K
dfuse-client.umd.js.gz 14K
dfuse-client.umd.js.map 179K
dfuse-client.umd.js.map.gz 47K

After:

dfuse-client.es5.js 95K
dfuse-client.es5.js.map 176K
dfuse-client.umd.js 48K
dfuse-client.umd.js.gz 14K
dfuse-client.umd.js.map 178K
dfuse-client.umd.js.map.gz 47K

@dawsbot dawsbot closed this Apr 13, 2020
@dawsbot dawsbot reopened this Apr 13, 2020
@dawsbot dawsbot changed the title Transpile umd to commonjs Babel transform everything for older browsers Apr 13, 2020
@maoueh
Copy link
Contributor

maoueh commented Apr 16, 2020

@dawsbot I checkout the branch, used yarn build and the spread operator was still present in the UMD build. Does not seem to work from my side. Here my steps:

$ git checkout umd-target-es5
$ yarn build
$ cat dist/dfuse-client.umd.js | grep -Eo  ".{15}\.\.\..{15}"

t s;function r(...e){if(!r.enable
ion(){const e=[...i.names.map(s),
t.log=function(...e){return"objec
g&&console.log(...e)},t.formatArg
 via the 'gql` ...`' call, then i

@dawsbot
Copy link
Author

dawsbot commented Apr 16, 2020

Great catch @maoueh! I certainly missed that, let me take a look again. We're about to do a production deploy, so if we can solve this soon, we can put it to the test immediately 🔬

EDIT: I've given this a fair effort and cannot seem to transpile away the ... operators. The quirkiest thing to me is that only the amd bundle contains this operator. The other bundles do not. I'm still working on this, but if you can give it a look I'd be appreciative @maoueh

EDIT EDIT: It looks like the remaining spread operator comes in from https://github.com/visionmedia/debug - yup that's where the problem is. They don't ship a browser-friendly version, so I'll need to build an isomorphic version of their package

@dawsbot
Copy link
Author

dawsbot commented Apr 17, 2020

After diving deeper, I see that this PR is not the proper fix for this problem. We've temporarily navigated around this issue by moving our use of dfuse to the server-side once-again. I'll make another PR or send ideas when there's an isomorphic debug library

@dawsbot dawsbot closed this Apr 17, 2020
@dawsbot dawsbot deleted the umd-target-es5 branch April 17, 2020 03:44
@maoueh
Copy link
Contributor

maoueh commented Apr 17, 2020

Babel 7 is able to transpile modules found in node_modules, it should be possible to achieve that.

The debug maintainer even suggest that and its the main reason they want to drop support for ES5 in the core library.

I have a local copy of your branch, I'll fix that, and I since it updates rollup, that was even more welcome PR.

@dawsbot
Copy link
Author

dawsbot commented Apr 17, 2020

Incredible, thank you for taking this and adding to it @maoueh! Can you tag me after on the diff or the PR so I can learn what the final diff ends up as?

@maoueh
Copy link
Contributor

maoueh commented Apr 20, 2020

@dawsbot Worked on this most of Friday afternoon and was not able to solve it. Rollup seems to be playing against me.

I'll need to revisit this later on. I did not drop the ball, will at least update the full config and everything.

I might decided so switch to Webpack also, which I know more than Rollup. I'll see.

@dawsbot
Copy link
Author

dawsbot commented Apr 21, 2020

Superb, thank you for taking a look on this. It's not fun to point out, but I think there will be several steps to getting this browser-ready. Here's what we've found:

  1. The es2015 syntax like ... in this bundle
  2. The Error.captureStackTrace bundled in by the debug module. See this on Sentry here. This is because debug does not ship an isomorphic package, they require any consumers to manually do a bundling process. https://sentry.io/share/issue/5cbf837461424861b8163cafd997765a/

You can build a browser-ready script using browserify, or just use the browserify-as-a-service build, if you don't want to build it yourself.

Luckily, we're no longer in a rush on this, we've moved dfuse entirely to the server-side so that users do not get this package in their browser bundles 👌

@maoueh
Copy link
Contributor

maoueh commented Apr 21, 2020

While the library is indeed not isomorphic in the strict sense of the actual definition, the debug library is still browser friendly, it has a special implementation for browser support: https://github.com/visionmedia/debug/blob/master/package.json#L50.

The UMD bundle I create use this special browser entry file (see https://github.com/dfuse-io/client-js/blob/master/rollup.config.js#L28). The browser version of debug is indeed ES6 code. Even my bundle is ES6 and not ES5, so that's something I'll need to either fix or emphasis on the documentation

If you had a problem with captureStackTrace, it's probably because the configuration on your side when bundling did not pick the correct dependency version of debug module. Not sure what you were using, you should ensure to always configure all pulled dependencies to use the browser field of the package.json if available and fallback to module then main. This is usually a configuration that need to be done on the bundler tool itself.

And while working on the issue, I noted another inconsistency in my build pipeline. The module version file is named dfuse-client.es5.js but it's really more a dfuse-client.es6.js, since the target compilation of the TypeScript code is set to es6.

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.

Remove spread operator from umd bundle
2 participants