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

Include a UMD build #44

Closed
ifiokjr opened this issue Oct 3, 2018 · 17 comments
Closed

Include a UMD build #44

ifiokjr opened this issue Oct 3, 2018 · 17 comments
Labels
help wanted Extra attention is needed

Comments

@ifiokjr
Copy link

ifiokjr commented Oct 3, 2018

I'm having to transpile the module before usage and I'm not sure if that's intentional.

This is the line which caused me pain.

ky/index.js

Line 192 in eb4b341

export default createInstance();

Feel free to close this is if the use of export is intentional, I couldn't really tell from the docs.

@rifler
Copy link
Contributor

rifler commented Oct 4, 2018

run into the same problem when using ssr.
Have you thought about using main (with commonjs exports) and module (with es6) fields in package.json?

@rifler
Copy link
Contributor

rifler commented Oct 4, 2018

quick fix
add next line in scripts section of your package.json:

"prepublish": "babel ./node_modules/ky/index.js --out-file ./node_modules/ky/index.js --config-file=./.babel-node.config.js"

And the .babel-node.config.js file content:

module.exports = {
    presets: [
        [
            '@babel/preset-env',
            {
                loose: true,
                targets: {
                    node: true,
                },
            },
        ],
    ],
};

@callumlocke
Copy link
Contributor

Seems to have been intentional: #23

But FWIW this is a confusing decision. Requiring a fetch polyfill is one thing, but requiring transpilation of a script in node_modules is pretty unusual and is likely to make using Ky infeasible for a lot of people. I'm not saying it's the wrong thing to do, but it's the wrong thing to do in a minor bump without a bit more explanation and warning.

@ironblock
Copy link

I agree with all of those points. I feel like the only thing "standard" about modules is that TC39 stopped arguing about them.

Of course, it's worth noting... The README specifically states:

Browser Support
The latest version of Chrome, Firefox, and Safari.

So, if you want to be pedantic, this warning was theoretically given. Technically, all of those browsers "support" modules now. Of course, they do it in a variety of ways, and in my opinion you'd be a raving lunatic to try and use them without the comfort and safety of Babel.

I think someone (who isn't me) could also make the argument that all packages on npm should be the most modern possible syntax and the end user is responsible for transpiling "backwards". As in, you should "opt in" to supporting legacy environments (and Node).

@sindresorhus
Copy link
Owner

but it's the wrong thing to do in a minor bump without a bit more explanation and warning.

Per semver, minor bumps in pre-1.0.0 versions are to be considered like major bumps. That's also why ^0.1.0 doesn't match 0.2.0.

@sindresorhus
Copy link
Owner

I think someone (who isn't me) could also make the argument that all packages on npm should be the most modern possible syntax and the end user is responsible for transpiling "backwards".

Yes, that would be me.

@sindresorhus
Copy link
Owner

sindresorhus commented Oct 5, 2018

The problem is that there's no standard way to expose both a file with modern syntax and ES5 syntax. The common pattern is main and module field in package.json. The problem is that most tools expect the module field to mean ES5 with import/export syntax. So there's really no field for "modern syntax".

See: https://babeljs.io/blog/2018/06/26/on-consuming-and-publishing-es2015+-packages

Would having a require('ky/es5') be enough? If so, I'm willing to do that, but I don't think we can reasonably implement something automatic. And what would the babel-preset-env browser config be?

This was referenced Oct 5, 2018
@sindresorhus sindresorhus changed the title SyntaxError: Unexpected token export Include a ES5 build Oct 5, 2018
@callumlocke
Copy link
Contributor

callumlocke commented Oct 5, 2018

+1 to keeping ES6 as the first-class entrypoint to encourage progress, and also providing a 'legacy' entrypoint like require('ky/es5') (or require('ky/umd')?) to make it easy for most people to keep using Ky in the same way they use most other npm packages.

As for what the Babel preset-env config should be... I guess there's no perfect answer, but I think pragmatically it makes sense to err on the 'generous' side and support plenty of old browser versions (e.g. IE9+). There's little point being minimalist, because the whole point is to create a drop-in entrypoint that just works. For anyone who cares about bundle size enough to be concerned, they can use the ES6 version and compile it however they want. Also, in practice there won't be much difference in Ky's output size whether it supports IE11 or IE9.

@sindresorhus
Copy link
Owner

(or require('ky/umd')?

👍

@sindresorhus
Copy link
Owner

Alright. PR welcome to add the Babel config. The config should be in package.json.

@sindresorhus sindresorhus added the help wanted Extra attention is needed label Oct 5, 2018
@callumlocke
Copy link
Contributor

Off topic, just curious, why do you prefer it to be in package.json?

@sindresorhus
Copy link
Owner

I prefer to keep all config in one file whenever possible.

@ironblock
Copy link

ironblock commented Oct 5, 2018

I realize now that "modern syntax" was maybe the wrong thing for me to have said, since that's a pretty flexible definition. Do you have any rules in mind for what syntax should be encouraged/supported? Only things ratified into the actual language specification, or is anything shipped by a browser vendor safe?

As background, I think I had the same use case as @rifler - I wasn't trying to use ky in Node, but I have a SSR React application that bails out in Node due to the export statement. Since it was commonJS before, it "just worked" for my particular use case.

And, for my part, I think it's totally reasonable to not support old browsers (or Node). I - and probably a lot of other people - take it for granted that everything in npm is conventionally minified ES5, but the more I think about it... There's no reason that needs to be the case anymore. I think the addition of the es5 version is a good compromise, but it feels selfish in retrospect. 😄

Kinda off-topic: I'd love to see some kind of beautiful union of ky and got in the future, too. I bet I could get 90% of the way there with ky and isomorphic-fetch... 🤔 🤔 🤔

@LitoMore
Copy link

LitoMore commented Oct 8, 2018

For another point, what about using the Webpack for a min file?

Like this?

const path = require('path');

module.exports = {
	mode: 'production',
	entry: path.join(__dirname, 'index.js'),
	output: {
		path: path.join(__dirname, 'dist'),
		filename: 'ky.min.js',
		library: 'ky',
		libraryExport: 'default'
	}
};

@sindresorhus
Copy link
Owner

@LitoMore No, that's too much.

@rgehan
Copy link

rgehan commented Oct 23, 2018

I'm using ky in a NextJS app and had to add the following script to my package.json scripts (variation of the one posted by @rifler)

"postinstall": "babel ./node_modules/ky/index.js --out-file ./node_modules/ky/index.js --config-file=./.babelrc"

I also had to install this packages for this to work:

"@babel/cli": "^7.1.2",
"@babel/core": "^7.1.2",
"@babel/plugin-proposal-class-properties": "^7.1.0",

@sindresorhus
Copy link
Owner

#46 was closed for lack of activity. The required work is documented in #46 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

7 participants