Avoid CommonJS in Cycle DOM codebase

[staltz]

To better support tree-shaking tools that assume ES2015 modules.

[thiagoarrais]

Can you provide some more context here? Maybe an instance of CommonJS usage that you want to get rid of...

[jvanbruegge]

basicly replace any require call with es6 imports

basicly just cloning the repo, grepping for require (i suggest the_silver_searcher), editing those files

[thiagoarrais]

Thanks, @jvanbruegge! I did a quick grep and it seems like there are more than 14k occurrences.

I'll see if I can come up with a sed script or something and report back.

[thiagoarrais]

btw, I don't know if github allows it, but... Can you assign this issue to me?

[jvanbruegge]

you have to limit the search to src folders

[thiagoarrais]

Indeed: most occurrences of require were inside dom/node_modules. I couldn't find any under dom/src, though.

Maybe this issue is already solved?

[thiagoarrais]

Can't find any occurrences of exports either...

[thiagoarrais]

Did a git bisect and the last require statement went down with commit f459ada. This issue is probably stale.

[jvanbruegge]

did a quick ag search, this is the result:

ag require --ignore node_modules --ignore examples/ --ignore yarn.lock --ignore docs --ignore test --ignore lib --ignore package.json             
html/src/makeHTMLDriver.ts
5:const init: Init = require('snabbdom-to-html/init');
6:const modulesForHTML: ModulesForHTML = require('snabbdom-to-html/modules');

jsonp/src/index.ts
4:import jsonp = require('jsonp');

time/src/mock-time-source.ts
14:const combineErrors = require('combine-errors');

time/src/scheduler.ts
2:const makeAccumulator = require('sorted-immutable-list').default;

time/src/time-driver.ts
11:const requestAnimationFrame = require('raf');
12:const now = require('performance-now');

time/src/assert-equal.ts
3:const variableDiff = require('variable-diff');

time/src/run-virtually.ts
1:require('setimmediate');

(I removed the ones that were bad matches (mostly markdown)
I also did not find any exports statements

[thiagoarrais]

Oh, right... I thought the issue was limited to cycle dom...

Will take a look at that!

[thiagoarrais]

So far I've tried changing

const combineErrors = require('combine-errors');

to

import combineErrors from 'combine-errors';

As I understand, this is the equivalent CommonJS/TS syntax for importing the default export from combine-errors and naming it combineErrors. But I keep getting this error:

Could not find a declaration file for module 'combine-errors'

I'm not sure what I'm supposed to do here. Judging from the Typescript docs it seems like one can add a .d.ts file to some kind of type definitions repo so that it becomes possible to say

npm install --save @types/combine-errors

I'm still learning both ES6 and TS. Maybe I'll be able to come back to this once I level up some more.

If anyone else wants to grab this, please do.

[SteveALee]

@thiagoarrais

To import a commonjs default export in typescript the format is actually neihter that you tired :) See the modules docs for TS

import combineErrors = require('combine-errors');

The error indicate the file cannot be found and as you did't prefix your file path with .\ I'm pretty sure it will be copying node lookup and it will looking for the file in node_modules. Perhaps that is not correct?

If the file is is plane js then yes, you will need to provide a .d.ts to give what they call "ambient declations" for the types. There are various approaches for doing that. but if it is a npm module try making types@combine-errors a dependency. If that is part of cycle (or extras) the types are there

[thiagoarrais]

In this specific case, combine-errors is an external npm module. Here it is. I guess this is also the case for every other occurrence of CommonJS requires. I've also tried to install the corresponding @types depencency with no success...

😞

[thiagoarrais]

Taking a step back, how can I test the use of tree shaking tools with Cycle.js? I've found the webpack guide, but I can't easily see how to use it.

[jvanbruegge]

There is the bundleAnalyse plugin and the stats plugin. The stats can be used to see the module dependencies. See my polyfill WIP PR

[jvanbruegge]

But tree shaking is broken ATM, also see the PR

[thiagoarrais]

How can we demonstrate that tree shaking is broken? Can you give me a one-liner for that? (Remember I'm a JS/TS newbie)

[jvanbruegge]

Import the time driver from @cycle/time, webpack removes the mockTimeSource but still pulls in all dependencies of the mockTimeSource

[staltz]

So I guess this is done now?

[micahscopes]

@staltz I don't think this is fixed. I'm having trouble with a very basic rollup project, even when using rollup-plugin-commonjs. The culprits are those require calls, e.g. https://github.com/cyclejs/cyclejs/blob/master/time/src/scheduler.ts#L2

This stack overflow question provides a succinct description of the problem: https://stackoverflow.com/questions/47329214/rollup-js-typescript-and-require

[Widdershin]

@micahscopes: I have made a PR that removes all usages of require from the @cycle/time codebase. #761

After that is merged hopefully Rollup will work.

[Widdershin]

@micahscopes @cycle/time@0.11.0 is now released with no more require calls, let me know if this solves your problem 😄