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

add browsers field #288

Merged
merged 2 commits into from
Aug 28, 2016
Merged

Conversation

donaldpipowitch
Copy link
Contributor

This is a real fix for #117 to prevent webpack build warnings. Webpack will automatically use the browser build (see https://webpack.github.io/docs/configuration.html#resolve-packagemains).

@epoberezkin
Copy link
Member

I am not sure why adding "browser" field breaks the tests. I thought it was some dependency update, but then the following PR passed... Does it pass when you run them locally? Is it maybe breaking browserify?

@epoberezkin
Copy link
Member

@donaldpipowitch
Copy link
Contributor Author

It correct that browserify uses the browser field just like webpack. But it should be the correct way to deploy the browser build. If the CI task breaks it probably means that dist/ajv.bundle.js would break in the same CI task, too.

I'll try to run the tests locally.

I could use a webpack field instead of browser. That also works for webpack users and probably wouldn't break your CI job. I'm not a browserify user, but as far as I understand browserify they would want a browser field, too?

@donaldpipowitch
Copy link
Contributor Author

Tests run locally when using webpack. It fixes the warnings with #117. We can definitely start with the webpack field and if any browserify user needs a browser field we can switch to it later and fix the CI tasks.

@epoberezkin
Copy link
Member

I'm not a browserify user, but as far as I understand browserify they would want a browser field, too?

If you want to use the bundle it can be just loaded separately. You can either use browserify to create this bundle (that's what is used here) or you can bundle Ajv as part of your application, in which case you just use var Ajv = require('ajv').

I am not sure I understand what would be your code to use Ajv if you include the bundle rather than the index file - the bundle doesn't return the Ajv constructor as far as I understand. Would you just use global that the bundle exposes? In which case I don't think I understand why you need webpack at all if you use the bundle. And if you are going to use as above I don't think it would work.

@donaldpipowitch
Copy link
Contributor Author

Isnt the bundle a UMD file? Than it would return the constructor. It must be that way as I can use Ajv just fine with the bundled file.

"why you need webpack at all if you use the bundle" Well, I use the bundled file of Ajv, but my application has a lot of other dependencies and my own source code which wants to be bundled, too. Right? ;) And why would I want a prebundled file of one my dependencies? Well, to get rid of all the warnings from requires in try/catch-blocks for optional dependencies :)

@epoberezkin
Copy link
Member

Got it. Thank you!

@epoberezkin epoberezkin merged commit 8150102 into ajv-validator:master Aug 28, 2016
@donaldpipowitch
Copy link
Contributor Author

Thank you!

@donaldpipowitch donaldpipowitch deleted the patch-1 branch August 29, 2016 08:55
@epoberezkin
Copy link
Member

In 4.6.0

@asprouse
Copy link
Contributor

asprouse commented Sep 1, 2016

Webpack still complains

WARNING in ./~/ajv/dist/ajv.bundle.js
Critical dependencies:
1:476-483 This seems to be a pre-built javascript file. Though this is possible, it's not recommended. Try to require the original source to get better results.
 @ ./~/ajv/dist/ajv.bundle.js 1:476-483

Seems like the best way to solve this is just to pass these dependencies in via the options.

@donaldpipowitch
Copy link
Contributor Author

Interesting. I don't get this warning. But it is a different warning and it is just one, not three. Hmm...

@epoberezkin
Copy link
Member

Is it possible instead to add IgnorePlugin to package.json somehow? Or add some other file that webpack would take into account and would use the plugin? I am not a webpack user, just thinking about what could work.

@epoberezkin
Copy link
Member

Wouldn't the better solution be to simply add to docs the instruction to use IgnorePlugin (see #117 (comment))?

@epoberezkin
Copy link
Member

In which case users who use any of those packages could also have them included by changing the regexp?

@asprouse
Copy link
Contributor

asprouse commented Sep 2, 2016

I think that at least for now there should be some documentation. I am not using the async features in my app so my workaround is to import Ajv from 'ajv/lib/ajv'; and to use the following in my webpack config:

new webpack.IgnorePlugin(/^regenerator|nodent|js\-beautify$/, /ajv/),

I think that the choice of whether to include nodent or regenerator on the frontend is not one that should be hidden to the user because they are pretty large. I almost abandoned using Ajv on the frontend (after using it on my backend) because I thought I need to add another ~300kb (uncompressed) to my bundle. I would propose the following to sidestep this problem all together:

import Ajv from 'ajv';
import regenerator from 'regenerator';

const ajv = new Ajv({ regenerator });

This would simplify the configuration over using transform: 'regenerator' and async: 'es7' and make it more explicit about what you are actually adding to your bundle when you use Ajv. Either way you still need to add it to package.json. Bundled dependencies only seem useful if you managing your dependencies with script tags.

Anyway that's my 2 cents. Sorry if this is off topic to this pull request.

@donaldpipowitch
Copy link
Contributor Author

donaldpipowitch commented Sep 2, 2016

I'd like to not include framework specific configuration to my webpack configuration. I share between a lot of (compex) projects and so far it works without specific configurations. I thought the browsers field would be the idomatic way to load a browser specific build from a framework both for webpack and browserify. (I also thought This seems to be a pre-built javascript file. is mostly about minified files.)

@sokra Is there a way to rewrite this line so it is ignored by webpack, but usable by browserify? (Sorry, for pinging you directly.)

@sokra
Copy link

sokra commented Sep 5, 2016

Use the browser field it you want different modules for browser and server.

@loris
Copy link

loris commented Oct 6, 2016

So what is the correct configuration to avoid This seems to be a pre-built javascript file. webpack warning?

@Aaronius
Copy link

In our case we did:

module: {
    // Without this webpack will log a warning.
    noParse: /node_modules\/ajv\/dist\/ajv.bundle.js/
}

And it seems to be working fine. From my understanding, ajv wants us to use dist/ajv.bundle.js when we're building for the browser and dist/ajv.bundle.js is built using webpack/browserify/whatever so using noParse seems appropriate. I guess.

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

Successfully merging this pull request may close these issues.

6 participants