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

Use tsdx to build without transpiling generators #120

Merged
merged 11 commits into from
May 18, 2020
Merged

Use tsdx to build without transpiling generators #120

merged 11 commits into from
May 18, 2020

Conversation

taras
Copy link
Member

@taras taras commented May 16, 2020

Motivation

We made a decision to ship generators in our distribution bundles because IE11 compatibility is not important to us. It's surprisingly difficult to get this to work. We tried using microbundle but that turned out to be even more complicated because their modern and cjs formats have mutually conflicting configuration.

Approach

I tried tsdx but it also transpiles generators by default. There is an RFC to make tsdx more customizable but in the mean time I patched tsdx to exclude @babel/plugin-transform-regenerator from Babel plugins using patch-package.

  1. Added patch-package as a dependency in root and run patch-package on postinstall
  2. Added @babel/preset-modules to effection dependencies because for some reason it started complaining that it was missing (it might be the result of an upgrade that was caused by tsdx installation)
  3. Updated events to prepack with tsdx
  4. Added module entry to events because it's also used in the web
  5. Updated node to prepack with tsdx
  6. tsdx uses rollup under the hood which requires that modules use ES6 syntax instead of commonjs. So I added module: "esnext" to events and node tsconfig.

Learning

patch-package is pretty cool. It has a nice workflow. When you need to change a dependency, modify it's code in node_modules and run npx patch-package <package_name>. It'll compare a fresh copy of the package that you're modifying to your changed copy and created a diff. It'll automatically apply this diff when you run yarn.

@taras taras requested a review from cowboyd May 16, 2020 16:16
@github-actions
Copy link
Contributor

github-actions bot commented May 16, 2020

The preview packages of this pull request have been published.
Click on the following packages for instructions on how to install them:

effection

Install using the following command:

$ npm install effection@try-rollup

Or update your package.json file:

{
  "effection": "try-rollup"
}

@effection/events

Install using the following command:

$ npm install @effection/events@try-rollup

Or update your package.json file:

{
  "@effection/events": "try-rollup"
}

@effection/node

Install using the following command:

$ npm install @effection/node@try-rollup

Or update your package.json file:

{
  "@effection/node": "try-rollup"
}

Generated by 🚫 dangerJS against 66d8db0

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Weird that the type tests are failing because of a mocha issue. I wonder if there is some type declarations in the node_modules directory that are getting included that weren't getting included before.

patches/tsdx+0.13.2.patch Show resolved Hide resolved
@taras taras requested a review from cowboyd May 17, 2020 08:47
Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Should we try linking it against bigtest before merging? I'd be happy to help out with that.

@@ -1,4 +1,7 @@
{
"extends": "./tsconfig",
"compilerOptions": {
"module": "esnext"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the default in @frontside/tsconfig or does it say?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It defaults to module: "commonjs"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a side discussion, but should it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure, probably not.

@cowboyd cowboyd merged commit aedd0f4 into master May 18, 2020
@cowboyd cowboyd deleted the try-rollup branch May 18, 2020 19:23
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.

2 participants