Aaronius / penpal

A promise-based library for securely communicating with iframes via postMessage.
MIT License
399 stars 56 forks source link

Target es5 #48

Closed willrnch closed 4 years ago

willrnch commented 4 years ago

When targeting es6, the build uses import export style module. Therefore, when including penpal in a project using jest, the code must be transpiled first.

willrnch commented 4 years ago

Or could you provide an example that works with create-react-app? Run npx create-react-app my-app --template typescript. Change App.tsx by something like:

import React from 'react';
import { ErrorCode } from 'penpal';

export default () => (
  <div>
    {ErrorCode.ConnectionDestroyed}
  </div>
);

run yarn test and see that it fails with:

/node_modules/penpal/lib/index.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import _connectToChild from './parent/connectToChild';
                                                                                                    ^^^^^^^^^^^^^^^

    SyntaxError: Unexpected identifier
Aaronius commented 4 years ago

@willrnch thanks for the PR. Whether to still transpile npm packages to ES5 is a hotly debated issue. I'm still on the fence about it, but if I were to do it I would include both es6 and es5 variants. Until then, add this to your package.json:

"jest": {
  "transformIgnorePatterns": [
    "node_modules/(?!penpal)"
  ]
}
willrnch commented 4 years ago

I have been through almost every single solution mentioned here. The only one that worked was to use ts-jest. Now I am facing the same issue with ts-node. I will try to replace it by babel-node.

willrnch commented 4 years ago

Until ES6 modules get stable: penpal-es5.

Aaronius commented 4 years ago

@willrnch, what error were you getting with the solution I proposed? I ran it on a new create-react-app and it seemed to work fine.

Aaronius commented 4 years ago

For reference to my future self or others that stumble upon this, here are the related Jest issues that are tracking ES module support:

https://github.com/facebook/jest/issues/4842 https://github.com/facebook/jest/issues/9430

markerikson commented 4 years ago

Note that Create-React-App transpiles packages from node_modules, but that's about the only tool that does so.

I just tried using this with Next.js, and got the exact same kind of error:

export { default as connectToChild } from './parent/connectToChild';
^^^^^^

SyntaxError: Unexpected token export
    at Module._compile (internal/modules/cjs/loader.js:723:23)

Given the current state of the ecosystem, yes, compiling to ES5 as part of publishing is still the standard approach. I know there's some weird shenanigans you can do to define multiple entry points based on bundling and such. Please rework your publishing to do ES5 first and handle other options as well.

Penpal looks like it fulfills a number of my criteria (maintained, simple, TS typings, etc), but at the moment it doesn't look like I can use it in my app due to this.

update

I was able to get this working with Next.js specifically:

// next.config.js
const withPlugins = require('next-compose-plugins');
const withTM = require('next-transpile-modules')(['penpal']);

module.exports = withPlugins([withTM]);

Having said that, I'd still request changing the publish step to target ES5/CommonJS as the standard syntax, and include other formats as well in the published archive with appropriate package.json module loading references.

For what it's worth, we use the TSDX build tooling package to publish Redux Toolkit, and it works great for us. Here's what our published package contents look like:

https://unpkg.com/browse/@reduxjs/toolkit@1.4.0/

Aaronius commented 4 years ago

Alright, I concede. I've added es5 transpilation to v5.1.0, which I just published. es6 should automatically be used when supported by the consumer's bundler, otherwise es5 should automatically be used. Please give it a try and let me know if you run into any issues.

willrnch commented 4 years ago

I get this error using typescript:

TS7016: Could not find a declaration file for module 'penpal'. '/node_modules/penpal/es5/index.js' implicitly has an 'any' type.
  Try `npm install @types/penpal` if it exists or add a new declaration (.d.ts) file containing `declare module 'penpal';

Can you configure the build so the es5 directory also contains the .d.ts files?

Aaronius commented 4 years ago

@willrnch Published in v5.2.0. Thanks for bringing that to my attention.

Aaronius commented 4 years ago

@willrnch, I modified how this is done in v5.2.1. Rather than making duplicate type files in the es5 directory, I just used the types field in package.json to point to the types in lib. This seems to work well, but let me know if it gives you trouble.

Aaronius commented 4 years ago

Also, I'm still new to dealing with type declarations, so I'm trying to balance what qualifies as a breaking change. This may be one of those things that should have been considered a breaking change if developers were importing type files directly from the es5 directory.