build stuff

[brainkim]

Fixes #87 #89
This PR changes the build outputs slightly so the package supports more environments. It is scheduled for the first 0.2 release.

• Moves esm files directly to the root.
• Adds export map.
• Adds UMD build.
• Changes output formats for builds to not transpile generators/async stuff.

[brainkim]

why jest

[brainkim]

Changes the github command to only test against 13. @ryhinchey Is this okay? There’s nothing about Crank which makes testing it against different node versions necessary and I don’t feel like having two jest config files for the CI. Why does node have to be like this though.

[dfabulich]

This PR fixes #87 and it fixes #89 in Node 13.2+.

It doesn't/can't fix #89 in Node 12, but the only way to fix #89 in Node 12 would be to turn the root dom.js and html.js files into CJS files, which isn't worth it, IMO.

There’s nothing about Crank which makes testing it against different node versions necessary

I think this PR in particular absolutely does benefit from testing various Node versions, but hopefully PRs like this will be rare.

I note that the PR also adds a UMD build. I have a bug to report in the UMD build, and I think this bug will annoy you, so please wear peril sensitive sunglasses before reading the bug report.

UMD bug

The UMD build currently doesn't work in Node, any version, because it's using a .js file extension, but the root package.json says "type": "module".

$ node umd.js
file:///private/tmp/crank/umd/umd.js:4
    (global = global || self, factory(global.Crank = {}));
                        ^

ReferenceError: self is not defined
    at file:///private/tmp/crank/umd/umd.js:4:25
    at file:///private/tmp/crank/umd/umd.js:5:2

This is happening because rollup-generated UMD scripts are meant to be interpreted as CJS scripts in Node. See the preamble definition in umd.js:

(function (global, factory) {
    typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports) :
    typeof define === 'function' && define.amd ? define(['exports'], factory) :
    (global = global || self, factory(global.Crank = {}));
}(this, (function (exports) { 'use strict';

The preamble function first attempts to detect CJS, then falls back to AMD (but who uses that anymore?) and finally defaults to setting a global in an IIFE. The problem is that it's trying to use this || self as the global; this is defined in CJS and browsers but not in ESM; self is defined in browsers, but not in Node.

I believe the easiest fix would be to use the packageCJS plugin in the umd directory.

[brainkim]

Tested on node 10, 11, 12, 13 and 14. It all still seems to work. Parcel and webpack seem to work too. In node12 you have to explicitly require the cjs directory, but that’s something I don’t care about.

@dfabulich I actually turned the umd build into a single file umd in the root, because there isn’t really a point in doing multiple umd files anyways, given that everything has to be exported as a single object. This was mainly to satisfy an exotic environment requirement by @fritx (#45) and I assume if they’re using node they can just use the cjs files.

I also moved the esm files directly to the root. This should make working with all the new npm to esmodules bundlers a little happier.

[brainkim]

Also rollup-plugin-ts by @wessburg is likely the first build plugin that hasn’t required a half-day to install and configure. It just worked! It’s so rare to see this in build tooling just wanted to give a shoutout.

[dfabulich]

FYI, moving the files out of the esm directory does not make anything easier with Snowpack 2 integration. (Does it actually help with Parcel or Webpack? I don't see how.)

IMO, the UMD build should go back in the umd subdirectory and add a package.json that says {"type": "commonjs"} so that it works in Node. Otherwise, the UMD build is just a browser-only IIFE build; the whole point of UMD is that U stands for universal; it's supposed to work in CJS, AMD, and in the browser.

[brainkim]

FYI, moving the files out of the esm directory does not make anything easier with Snowpack 2 integration. (Does it actually help with Parcel or Webpack? I don't see how.)

@dfabulich Ahhh I guess I just misunderstood the nature of the snowpack bugs. Having the files built directly into the root of the package just feels a little cleaner to me, as opposed to having dummy files in the root which link to the esm builds. Not having to do that extra hop from index.js to esm/index.js is nice for stack traces and unpkg and such.

What do we have to do for snowpack integration? Did you get any response from those people? I’m feeling frisky so I might, maybe, just maybe be willing to create dummy directories and package.json files to get dom and html to work.

Thank you for all the feedback on this stuff. I literally did not have the patience to try and figure it out on my own so this is incredibly helpful 🧡

[brainkim]

IMO, the UMD build should go back in the umd subdirectory and add a package.json that says {"type": "commonjs"} so that it works in Node. Otherwise, the UMD build is just a browser-only IIFE build; the whole point of UMD is that U stands for universal; it's supposed to work in CJS, AMD, and in the browser.

FINNEEEEEEEEEEEEEEE

[dfabulich]

Merge this!

[brainkim]

@dfabulich Are you confirming it all works? I want to do one last 0.1 release with the performance fixes so 0.1 dependents don’t all perform like ass, and then have this merged for the first 0.2 release. At the same time, I’m also thinking what if we just don’t consider this stuff a breaking change. It is a beta after all. Probably best to make it 0.2 to be responsible.

[dfabulich]

Yes, it all looks good to me, and I agree that calling it 0.2 is wise. It's definitely some kinda breaking change, because it's no longer possible to require random, arbitrary files in the package.

[ryhinchey]

Changes the github command to only test against 13. @ryhinchey Is this okay? There’s nothing about Crank which makes testing it against different node versions necessary and I don’t feel like having two jest config files for the CI. Why does node have to be like this though.

@brainkim do the tests only run in CI in node 13? That doesn’t make sense to me but I’d say right now it’s not a big deal. The yaml file can be updated to only run certain commands in a specified node env so we’ll want to do that at some point in the future

[brainkim]

@ryhinchey the problem is that turning type into "module" then requires us to have jest.config.js be an esmodule.

[ryhinchey]

@ryhinchey the problem is that turning type into "module" then requires us to have jest.config.js be an esmodule.

I see. I don’t have the knowledge or time right now to propose an alternative. The only thing that truly needs to be tested on different node versions is the String renderer and that can be done in a separate PR.

I’ll need to research what other packages that have type:module do for their config files.

[brainkim]

This is the error I see on node 12 with the module.exports config file.

yarn run v1.22.4
$ jest --config jest.config.js --color --no-cache
�[31mError: Jest: Your version of Node does not support dynamic import - please enable it or use a .cjs file extension for file /Users/brian/Projects/@bikeshaving/crank/jest.config.js�[39m
�[31m    at readConfigFileAndSetRootDir (/Users/brian/Projects/@bikeshaving/crank/node_modules/jest-config/build/readConfigFileAndSetRootDir.js:119:17)�[39m
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[brainkim]

Oh wait, we can make jest.config.js a cjs file and have everything work. Will lift the ban on cjs file extensions just for jest.

[ryhinchey]

Oh wait, we can make jest.config.js a cjs file and have everything work. Will lift the ban on cjs file extensions just for jest.

Yah. It all just feels so hacky.

[ryhinchey]

Not trying to throw a wrench in here but what’s the push to support esmodules in node in the first place? It seems to add a lot of complexity for little gain.

There are clear examples out there to support treeshaking with bundlers via esmodules and if that’s the goal, we can simplify all of this.

[brainkim]

@ryhinchey At the end of the day, all of the complexity is scoped to the rollup build and package.json, thanks to some copying of package.json files. I refuse to use mjs/cjs files on principle, and I’m very glad we found a way to not have to use them (besides the jest config) The exports map in package.json is definitely a huge footgun but we can just be careful about adding submodules.

[brainkim]

There are clear examples out there to support treeshaking with bundlers via esmodules and if that’s the goal, we can simplify all of this.

The goal was to get the package to work in node 12/13/14 without throwing cryptic errors. Treeshaking is a complex topic and I don’t actually think any code in Crank can or should be treeshaken.

[dfabulich]

I kicked this off because I wanted to fix #87, which seems like a worthy goal to me.

An alternate approach would be to put the ESM scripts back in the ESM folder, set the project type back to CJS, and drop a type: module package.json just in the ESM folder. That would avoid any Jest hacks. But I think the CJS build will most likely go away over time; converting the project to ESM seems like a better choice to me.

[brainkim]

But I think the CJS build will most likely go away over time; converting the project to ESM seems like a better choice to me.

Agreed. I think we should look to the future, and the future is probably ESModules everywhere. I want to see if we can get Crank working with Deno soon. I think Crank’s usage of generators and async generators meshes really nicely with Deno’s philosophy and feature set.

The last question I have before merging. Can we get away with not transpiling modules at all, for all the output formats? The justifications are:

1. There are significant performance penalties to transpiling generators and async functions
2. Crank doesn’t use any syntax features beyond async generators, and if you’re not in an environment which supports async generator functions, you’re already going to be doing a lot of transpilation anyways, and making people transpile Crank as well doesn’t seem like a big ask, given that it is a framework.
3. I’ve been careful not to use newer syntax features beyond async generator functions like decorators or private members.

[ryhinchey]

I think no transpilation is a great idea. Not worth the perf penalties and bundle size implications

[dfabulich]

What do you mean by "no transpilation"? Some kind of transpilation has to happen for TypeScript, right? (Or would you switch to JSDoc TypeScript?) https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html

"There are significant performance penalties to transpiling generators and async functions" … I assume you're referring to transpiling these to non-generator/non-async functions? Today's build doesn't do that, does it?

[brainkim]

Ah yes, to clarify, I mean using "target": "ESNEXT" in our tsconfig. Removes all the typescript types, but leaves generator functions and async functions untranspiled. So if you look at the build artifacts from previous releases they have code like:

HostNode.prototype[Symbol.iterator] = function () {
return __generator(this, function (_a) {
switch (_a.label) {
case 0:
if (!!this.unmounted) return [3 /*break*/, 2];
if (this.iterating) {
throw new Error("You must yield for each iteration of this.");
}
this.iterating = true;
return [4 /*yield*/, __assign(__assign({}, this.props), { children: this.childValues })];
case 1:
_a.sent();
return [3 /*break*/, 0];
case 2: return [2 /*return*/];
}
});
};

I thought we would do esnext for esm and es5 or es6 for the cjs and umd builds, but my current thinking is to do esnext for all environments.

[brainkim]

Did some thinking and I think it wouldn’t hurt to try leaving modules untranspiled. If people have trouble with this for their specific environments we can figure out on a case by case basis whether we should make them transpile or if we have to transpile the lib, and then even if we do have to make the change it wouldn’t be a breaking change anyways.

[palavrov]

Didn't follow the entire discussion and don't know how relevant will be, but if you need to modify package.js (or adding/removing/generaing files) during build/pack process could take a look to small package I did. (It's not production ready so no self promotion here ;) )

https://github.com/treedys/postpack

Piping and modifying the content of npm pack archive is a bit gray area and was a bit tricky to properly handle.

Again - if the comment is not relevant feel free to remove to avoid adding noise here.

[brainkim]

Pushing out the release of 0.2 to at latest Friday so I can implement some features and do some more performance experiments, this time with breaking changes to the undocumented renderer API. Let me know if there’s a specific reason you want it sooner.