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

Moment is no longer an optional dependency #814

Closed
mickelus opened this Issue Oct 18, 2018 · 9 comments

Comments

Projects
None yet
6 participants
@mickelus
Copy link

mickelus commented Oct 18, 2018

After updating from 1.7.0 to 1.8.0 we are no longer able to use pikaday without having moment installed. Is moment still indended as an optional dependency?

We get the following error in our webpack build:

WARNING in ./node_modules/pikaday/pikaday.js
Module not found: Error: Can't resolve 'moment' in '$project/node_modules/pikaday'
@ ./node_modules/pikaday/pikaday.js`

In our grunt/browserify build we get this error:

Running "browserify:release" (browserify) task
Error: Cannot find module 'moment' from '/node_modules/pikaday'

@niksajanjic

This comment has been minimized.

Copy link

niksajanjic commented Oct 22, 2018

And if this is not the bug but intentional change, then this is a breaking change and it should be labeled as 2.0 version.

@wojciechczerniak

This comment has been minimized.

Copy link
Collaborator

wojciechczerniak commented Oct 23, 2018

I see a warning, not an error.

In my test Webpack output is created correctly, without moment.js. With 1.8.0 you can ignore the warning or install moment.js. If you choose to install moment.js it will be included in the output and an optional dependency is met. When I switch back to 1.7.0 the moment.js is always installed and always included in the output file.

So now it's finally an optional dependency. Looks how it should work from the very beginning.

@saltcod

This comment has been minimized.

Copy link

saltcod commented Oct 23, 2018

#718 (comment)

^ worked for me.

@schoenkaft

This comment has been minimized.

Copy link

schoenkaft commented Oct 24, 2018

For Browserify, the only option I could find is running it with the --ignore-missing flag. To specify this in your config: ignoreMissing: true.

This will allow optional dependencies in a try/catch block to throw an exception without failing your build.

Depending on your setup this could be safe or unsafe (think of a production build needing to fail in a case like this). In my case it's safe, since any unresolved imports are caught early by ESLint (rule: import/no-unresolved).

@niksajanjic

This comment has been minimized.

Copy link

niksajanjic commented Oct 24, 2018

@schoenkaft Thanks for the idea, I have been using browserify and in case you don't have that flag, the build would fail.

Nevertheless, I still think this is a bug. If your code doesn't depend on moment.js there should be no error/warning whatsoever. Also, as @schoenkaft said, that flag is only a partial solution, because it can produce unsafe builds and shouldn't be considered a proper solution.

@x1c0

This comment has been minimized.

Copy link

x1c0 commented Oct 24, 2018

Same warning here building with Angular CLI: 7.0.2 / Node: 9.0.0 / Angular: 7.0.0

@niksajanjic

This comment has been minimized.

Copy link

niksajanjic commented Oct 24, 2018

After further investigation I realized this is an issue with browserify:
browserify/browserify#901

Therefore, as far as build from browserify goes, this isn't an issue of Pikaday. As a matter of fact, there is a try/catch inside Pikaday's source code that doesn't throw any errors or warnings, so this seems like an issue of build tools that we have been reported here.

@wojciechczerniak

This comment has been minimized.

Copy link
Collaborator

wojciechczerniak commented Oct 25, 2018

Thanks for sharing your findings @niksajanjic. It's a pitty that Browserify can't handle CommonJS try/catch optional require pattern. There is an --ignore-missing flag though. I guess that Webpack have an option to ignore the warning either.

Any ideas how we can handle this better? Should we? Might be better to invest this time into refactoring how moment.js dependency is handled, then mark it as breaking change and release 2.0.

@wojciechczerniak

This comment has been minimized.

Copy link
Collaborator

wojciechczerniak commented Oct 30, 2018

Duplicate of #815

@wojciechczerniak wojciechczerniak marked this as a duplicate of #815 Oct 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment