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

Fix Webpack issues in v4? #453

Closed
aikar opened this issue Apr 3, 2017 · 24 comments
Closed

Fix Webpack issues in v4? #453

aikar opened this issue Apr 3, 2017 · 24 comments

Comments

@aikar
Copy link

aikar commented Apr 3, 2017

Why are the require(name) pattern even used?

If I change it to require('nodent') for example, webpack stops complaining.

I feel a bit stuck in that we have no control over our ajv version in our product, as it's used by our dependencies. So it's not as simple as "use v5"

Can we backport the removal of "webpack: bundle.js blah" to v4 and change the requires to require('regenerator') instead of require('name') ?

@epoberezkin
Copy link
Member

epoberezkin commented Apr 3, 2017

@aikar 'nodent' and 'regenerator' are optional dependencies. If I make the change you suggest the Ajv bundle will be bigger by more than 1.2Mb (i.e. 1.3Mb minified instead of 110Kb minified).

Unfortunately Webpack doesn't seem to support optional dependencies in a way that would be compatible with nodejs/browserify. Browserify supports dynamic require when the package name is an expression and the bundle for that optional dependency is loaded separately.

So your options are:

  1. wait until version 5 of Ajv is released and dependencies are updated. It should happen soon, as soon as draft-06 of JSON-Schema specification is published. In v5 these optional dependencies are moved to ajv-async package, which most people don't need
  2. use Ajv bundle created with browserify and load it separately. It is available in npm package and via cdnjs. This bundle exposes global Ajv symbol
  3. use browserify instead of webpack
  4. fork Ajv and remove these optional dependencies completely (but you'll have to fork packages using Ajv as well)

@epoberezkin
Copy link
Member

By the way, is "webpack complaining" a real issue? I.e. does the bundle work? I was under the impression that it is possible to configure webpack to ignore these optional dependencies and it all works. If the only problem is some build warnings then it's not the problem that MUST be solved.

@aikar
Copy link
Author

aikar commented Apr 4, 2017

@epoberezkin that ignore line didn't seem to do anything for me in Webpack 2.3

Also, with the nature of webpack, unless you explicitly add code splitting to ajv, the files HAVE to be in the bundle if they were to work.

Could you maybe deploy a bundle without these deps into dist folder as part of v4, and then we can use the webpack resolve.alias feature to remap ajv to ajv/dist/webpack-no-async.js ?

@epoberezkin
Copy link
Member

The bundle is already deployed in dist folder in npm package

@elrumordelaluz
Copy link

@epoberezkin first, congrats for the great and useful lib.

Don't know if is related on this issue but I am using ajv@^5.0.4-beta as a separate chunk with webpack but obtains this:
screen shot 2017-04-04 at 18 47 38
If I require directly (so include in the main bundle) there's no error.

@epoberezkin
Copy link
Member

The bundle supplied with the npm package should work, it exposes a global Ajv variable.
I don't know what creates this Ajv.chunk.js, it's not in this package

@aikar
Copy link
Author

aikar commented Apr 5, 2017

@epoberezkin i think you misunderstood my request. I was asking for a file that either didnt include these deps, or changes them in a way that tricks webpack.

Ultimately, it's important to not get too complacent with ignoring critical sounding warnings (you know, the warning message kind of says the word critical), putting you in the mindset that when a more serious issue does arise, you end up ignoring it.

It also creates panic within the team. Sure i know its harmless, but when every one of my team mates pull the code down and see this new warning with the word 'critical', it creates a panic that the build lost stability.

I then have to explain everything about this to everyone.

Sometimes harmless messages cause more harm than expected.

@aikar
Copy link
Author

aikar commented Apr 5, 2017

Also per your suggestions, I am not a direct user of this, so I have no control how my dependencies load it. I ultimately need a build that doesn't trigger this that I can pin to bring the others into using it.

I'm curious to what even caused this issue, as 4.11.3 does not cause this, but 4.11.4 or 5 does.

cc @gajus, this issue is about why I asked for gajus/babel-plugin-react-css-modules#74

@gajus
Copy link
Contributor

gajus commented Apr 5, 2017

It also creates panic within the team. Sure i know its harmless, but when every one of my team mates pull the code down and see this new warning with the word 'critical', it creates a panic that the build lost stability.

Whats the actual error? I don't see any error message referenced in this thread that'd say "critical anything".

(I am not arguing against the issue. Just want to know whats the actual error.)

@gajus
Copy link
Contributor

gajus commented Apr 5, 2017

I'm curious to what even caused this issue, as 4.11.3 does not cause this, but 4.11.4 or 5 does.

Have you tried looking into the changes introduced between the two tags to identify what @epoberezkin change triggered the warning?

@aikar
Copy link
Author

aikar commented Apr 5, 2017

@gajus

WARNING in ./~/ajv/lib/compile/index.js
13:21 Critical dependency: the request of a dependency is an expression

WARNING in ./~/ajv/lib/async.js
96:20 Critical dependency: the request of a dependency is an expression

WARNING in ./~/ajv/lib/async.js
119:15 Critical dependency: the request of a dependency is an expression

note: I copied those from another closed ticket, so line #'s might not be right, as I skipped updating React Css Modules for now to avoid it. but the error message is the same as what i'm seeing.

But no I could not figure it out, my only suspect is the major version update to browserify v14.

@gajus
Copy link
Contributor

gajus commented Apr 5, 2017

Related issue webpack/webpack#196.

Whatever the change, Ajv is the subject of the recent discussion in that thread.

@epoberezkin
Copy link
Member

@gajus thank you. Added a question there: webpack/webpack#196 (comment)

@epoberezkin
Copy link
Member

I'm curious to what even caused this issue, as 4.11.3 does not cause this, but 4.11.4 or 5 does.

Have you tried looking into the changes introduced between the two tags to identify what @epoberezkin change triggered the warning?

@aikar I don't see what could have caused the difference between these versions. Are you sure there is no change in webpack configuration that happened at the same time?

@aikar
Copy link
Author

aikar commented Apr 6, 2017

Ok now i'm confused. It doesn't appear to be between those versions.

I rebuilt my yarn.lock even without updating BPRCM and locked ajv to 4.11.3 by adding it as a strict dependency to my project, and the issue came up still with this diff of versions:

https://gist.github.com/aikar/a464defedad78b02c327867791420299

I've verified I only have 1 copy of ajv in my node_modules.

@elrumordelaluz
Copy link

@epoberezkin where is exposed the file dist/ajv.bundle.js?
Found here and if I use it directly from a bundle in a local clone works perfect.

@epoberezkin
Copy link
Member

@elrumordelaluz The bundle is published in npm package. You need to npm install ajv. It's not in the repository.

@epoberezkin
Copy link
Member

ahmadnassri/node-har-validator#93 will fix it

@tizmagik
Copy link

This seems to still happen with ajv@5.1.5

@epoberezkin
Copy link
Member

what exactly?

@tizmagik
Copy link

Sorry, I meant the build warning for a require statement as a result of an expression, but it looks like there was an issue on my end not ajv@5 so never mind me! 😁

@epoberezkin
Copy link
Member

@brainstormtrooper
Copy link

OK... I can get rid of the warnings, but i don't think webpack can bundle everything that ajv is looking for... I'm using version 6 and I have a line like this in ./node_modules/ajv/lib/async.js
nodent = require(name)({ log: false, dontInstallRequireHook: true });...
the variable gives no folder context for webpack to work with, so how can it be bundled?

@epoberezkin
Copy link
Member

@brainstormtrooper There is no such file in ajv v6: https://github.com/epoberezkin/ajv/tree/master/lib

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

No branches or pull requests

6 participants