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

Target es5 #48

Closed
wants to merge 1 commit into from
Closed

Target es5 #48

wants to merge 1 commit into from

Conversation

willrnch
Copy link

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
Copy link
Author

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
Copy link
Owner

@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)"
  ]
}

@Aaronius Aaronius closed this May 18, 2020
@willrnch
Copy link
Author

willrnch commented May 18, 2020

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
Copy link
Author

Until ES6 modules get stable: penpal-es5.

@Aaronius
Copy link
Owner

@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
Copy link
Owner

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

jestjs/jest#4842
jestjs/jest#9430

@markerikson
Copy link

markerikson commented Jun 30, 2020

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
Copy link
Owner

Aaronius commented Jul 8, 2020

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
Copy link
Author

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
Copy link
Owner

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

@Aaronius
Copy link
Owner

Aaronius commented Aug 6, 2020

@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
Copy link
Owner

Aaronius commented Aug 6, 2020

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.

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.

3 participants