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

Package is not compatible with Create React App and it's testing setup #70

Closed
mucsi96 opened this issue Aug 10, 2019 · 10 comments
Closed

Comments

@mucsi96
Copy link

mucsi96 commented Aug 10, 2019

I see the published package contains import statements. This produce the following error in default create react app setup after running npm test

Test suite failed to run

    /home/igor/react-hooks-beer-example/node_modules/wouter/index.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import locationHook from "./use-location.js";
                                                                                                    ^^^^^^^^^^^^

    SyntaxError: Unexpected identifier

Can we publish a transpiled version containing require statements so no babel transpilation would be required inside node_modules?

@molefrog
Copy link
Owner

Good call! I actually was thinking about moving the package declarations back to CommonJS exports. And here is why:

  1. The ES6 module resolution for node_modules is still not standardized. You can use ES6 modules in the browsers now, but only for files that you know the exact location of, but you can't really write something like import { ... } from "wouter" in the browser.
  2. The ES6 modules are still aren't adopted without a flag in Node.js, which makes it hard to test. One of the main reasons we keep Babel as a dev dependency here in wouter is because we want to run Jest, and that creates an extra complexity in my opinion.

So I would rather have something that can easily be imported without a special setup in both setups until the ES6 modules are fully supported.

@mucsi96
Copy link
Author

mucsi96 commented Aug 12, 2019

Usually in package.json of popular packages I see "main" for require and "module" for ES6. See https://webpack.js.org/guides/author-libraries/#final-steps The first one points to the dist version with requires, the second also to dist version but with imports. Webpack is reading the later one and is doing tree shaking of es6 modules. But Jest is using the first one and expects no imports there

@mucsi96
Copy link
Author

mucsi96 commented Aug 12, 2019

Due to reason I was not able to use it in CRA I had to drop it and write an own minimal routing hook. Which is sad as I loved your package idea

@molefrog
Copy link
Owner

Looking good!

Does this work properly? I had issues with it and it seems like popState isn't triggered when the location changed through pushState:

Note that just calling history.pushState() or history.replaceState() won't trigger a popstate event. The popstate event will be triggered by doing a browser action such as a click on the back or forward button (or calling history.back() or history.forward() in JavaScript).

@molefrog
Copy link
Owner

Webpack is reading the later one and is doing tree shaking of es6 modules.

Right, tree shaking seems like a good reason to use ES6 modules.

@mucsi96
Copy link
Author

mucsi96 commented Aug 12, 2019

Check the source of the Link. Apart from doing a push state it triggers a pop state event. Then the implementation of router is very easy.

Yes ES6 is very good. But not for "main" property but for "module"

@molefrog
Copy link
Owner

molefrog commented Aug 16, 2019

Hey @mucsi96 I did some research and I think we could carry a folder called cjs/ in our project where we keep all CommonJS versions of our scripts (generated by the rollup on pre-publish).

We could also then point the main to cjs/index.js, however I just want to point out that this won't solve the problem for exports like const loc = require("wouter/use-location"). This is a known behaviour, there was a proposal somewhere specifies the mutiple entry points in package.json, but this is still work in progress.

@mucsi96
Copy link
Author

mucsi96 commented Aug 16, 2019

Yes that would be awesome. I think supporting users with CRA is much more important then exporting ES6 modules. Espetially that even its work in progress Webpack and rollup reads the module property for ES modules. So I don't see any isse by pointing main to cjs and ponting modules to current entrypoint

@molefrog
Copy link
Owner

@mucsi96 Hi, I've just released the latest version v2.2.1 which comes with proper CJS sources, located in the cjs/ folder. The package.json's main field now also points to the CJS source, so it should work fine in both NodeJS and Webpack, Rollup, CRA etc. (they will fetch the ESM sources instead).

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

No branches or pull requests

2 participants