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

Should include babel runtime with corejs as a regular dependency? #9

Open
cmdcolin opened this issue May 13, 2019 · 9 comments
Open

Comments

@cmdcolin
Copy link

If you install this library, it may give errors such as this

Module not found: Can't resolve 'core-js/modules/es.array.concat' in '/home/cdiesh/src/jbrowse-components/node_modules/react-hooks-async/dist'   

I believe this is due to not including @babel/runtime-corejs2 as a regular dependency (or maybe @babel/runtime and core-js: 3 as dependencies? not exactly sure)

@cmdcolin
Copy link
Author

maybe also consider react as peer dependency

@sibelius
Copy link

user should provide their own polyfill

import '@babel/polifyll'

@cmdcolin
Copy link
Author

If that is the case then this library should not add core-js polyfills via babel

@dai-shi
Copy link
Owner

dai-shi commented May 14, 2019

Yeah, it seems common that a library should not add polyfills, and let library users (developers) add them.

  • a) because there are various polyfills, and
  • b) you might want to use uncompiled src for only modern browsers or for compiling by yourself.

react as peer dependency

There it is already.

Closing this. Please feel free to reopen/open new one for further/another discussion.

@dai-shi dai-shi closed this as completed May 14, 2019
@durandj
Copy link

durandj commented Oct 9, 2019

I'm a bit confused about the outcome here. The decision was to not include polyfills in the build output but if you look at the dist directory you see that core-js is being imported to do polyfilling. This breaks the build for a web application I'm working on at work as anything not directly marked as a dependency gets removed and core-js isn't a dependency for react-hooks-async.
It sounds like using src is the only way forward for this which is unfortunate since it makes imports look very weird. Any chance this could be addressed?

@dai-shi
Copy link
Owner

dai-shi commented Oct 9, 2019

Thanks for the input.

anything not directly marked as a dependency gets removed

I'm not familiar with this. Is this a webpack config or something?

I would assume that one can add core-js in dependecies of their apps, but it's not your case, is it? Is it a project rule?

It sounds like using src is the only way forward

So, if you import src, does it work for you?

it makes imports look very weird.

You might be able to configure your bundler to use "module" before "main"?

Any chance this could be addressed?

Yeah, I think it's important so that most people can use this library without hassle.

@dai-shi dai-shi reopened this Oct 9, 2019
@durandj
Copy link

durandj commented Oct 9, 2019

Thanks for the fast response!

I'm not familiar with this. Is this a webpack config or something?
I would assume that one can add core-js in dependecies of their apps, but it's not your case, is it? Is it a project rule?

We use pnpm for our dependency management instead of yarn or npm. One of the rules that pnpm adds is that each package can only see the packages that it marks as dependencies. Because core-js isn't a dependency of react-hooks-async, you get compile time errors about missing dependencies. This problem also happens for peer dependencies but you can get around it by doing as you suggested and adding the peer dependency as a dependency to your package. However, for dependencies that aren't marked as a peer dependency and are missing, this won't work and causes the build error.

So, if you import src, does it work for you?

This does work but just looks fishy when I see that as the import path. If nothing else works I'll create an import alias to get around that, I would just prefer that that wasn't the optimal solution fro this case.

You might be able to configure your bundler to use "module" before "main"?

I'll see what I can do to make this happen as that sounds like it would make things easier.

@dai-shi
Copy link
Owner

dai-shi commented Oct 9, 2019

pnpm

I see. They have strict rules about dependencies.

This does work but just looks fishy when I see that as the import path.

If you can use the code in src instead of dist, it would always be better.
I totally agree that importing react-hooks-async/src is not nice.

You might be able to configure your bundler to use "module" before "main"?

I'll see what I can do to make this happen as that sounds like it would make things easier.

I'm not sure which bundler you are using, but webpack doesn't seem to allow specifying mainFields for specific packages. 😕


One option is to use peerDependenciesMeta and include core-js as an optional peer dependency.
Seems like pnpm supports it. pnpm/pnpm#1486

This is going to be a problem for people who use old versions of npm...
This is not the best solution either for your case, because the best solution is to use src nicely.

I wonder how other "async" libraries solve this problem. 🤔

@durandj
Copy link

durandj commented Oct 9, 2019

I was able to get things working using mainFields in my case (yay for small packages with few dependencies!).

I wonder how other "async" libraries solve this problem.

Great question, I'll see if I can dig up alternative solutions that could get around this problem.

Thanks for your assistance :)

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

4 participants