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

Consider to support ES "module" #87

Closed
jorgecasar opened this issue Feb 15, 2019 · 26 comments
Closed

Consider to support ES "module" #87

jorgecasar opened this issue Feb 15, 2019 · 26 comments
Labels

Comments

@jorgecasar
Copy link

It would be great to implement ES module entrypoint

@dougwilson
Copy link
Contributor

Sorry for my ignorance, what does that mean?

@jorgecasar
Copy link
Author

jorgecasar commented Feb 15, 2019

I mean EcmaScript Module syntax. To be able to use in Javascript modules like:

import { cookie } from 'cookie';

More info: import and export.

I will try to make a PR with that

@dougwilson
Copy link
Contributor

dougwilson commented Feb 15, 2019

Gotcha. I have seen folks doing import cookie from 'cookie'; without issue before. But I don't know the environment so cannot say what is different. They don't use the curly brackets as in your example, though.

If you do make a pull request, please be sure it includes tests and documentation, that would be most appreciated! Also any info you can provide written out that I need to know (not just long generic articles) so I can properly support any issues that arise from this in the future would be helpful too :)

@jorgecasar
Copy link
Author

I was reading and I guess that we need 2 stages: build and test. Also, we need to share the build result with the test stage (the pkg folder). We need a external storage as you can read on Travis: Data persistence between stages and jobs.

@wesleytodd
Copy link
Member

wesleytodd commented Feb 17, 2019

Hey, so I am not sure any PR should be made for this. When node and the browsers can land on a final path forward for ES Modules then maybe there will be something to do here (.mjs entry or something?). Until then, all of the compilation pipelines handle importing from a common js module. As @dougwilson points out above, import cookies from 'cokies'; should work just fine.

@jorgecasar
Copy link
Author

jorgecasar commented Feb 17, 2019

I'm trying to use it in the browser side with ES-modules and I get an error:

image

@dougwilson
Copy link
Contributor

Well, this is a Node.js module, so it doesn't work out-of-the-box in the web browser. You need to use browserify, webpack, etc. to create a browser version of this module for you to use. This would apply to pretty much every Node.js module out there.

@jorgecasar
Copy link
Author

That's why I would like to move to ES-modules and distribute different versions adding them in the package.json:

"esnext": "dist-src/index.js",
"main": "dist-node/index.js",
"module": "dist-web/index.js",

If the library has all ways final user doesn't need require other libraries to convert it.

@dougwilson
Copy link
Contributor

Gotcha. Well, that is very different, though, then changing the export type. For example, supporting the web browser means we actually need to test this module in the web browsers too, so we can support this. For example, if this module makes use of a Node.js API, then it still won't work in the web browser, right? And what about the pending PR that will add a dependency to this module, will that still work in the browser with your changes?

@jorgecasar
Copy link
Author

I'm using cookie-api-handler that it's an extension of rest-api-handler, a handler for REST APIs. That should work in the browser using module syntax.

I was looking for different approaches and I found cjs-to-es6 that you can run in the publish process to generate the es6 distribution.

I'm going to decline the PR until we can find the right solution.

@dougwilson
Copy link
Contributor

That module is using this module incorrectly. The parse function of this module is only made to parse the Cookie http header, but that module is passing in the Set-Cookie http header instead, which has very different syntax.

Perhaps that module should not even be using this module in the first place?

@jorgecasar
Copy link
Author

Maybe it's not crazy serve libs in several versions: My PR uses pika and NPM is considering to add it natively in NPM. npm/rfcs#35.

I guess it time to change the test process and build the distribution versions which will be tested in all Node versions.

@jorgecasar
Copy link
Author

Maybe I'm wrong, but the syntax it's not very different:

Set-Cookie: <name>=<value>[; <name>=<value>]...
[; expires=<date>][; domain=<domain_name>]
[; path=<some_path>][; secure][; httponly]

vs

Cookie: <name>=<value> [;<name>=<value>]...

What am I loosing?

@BorntraegerMarc
Copy link

I would love to be able to use this lib also in the browser!

@wesleytodd
Copy link
Member

@BorntraegerMarc, there is no reason you cannot use this in the browser today. Is there something specific you are looking for here?

@BorntraegerMarc
Copy link

well @dougwilson wrote:

For example, supporting the web browser means we actually need to test this module in the web browsers too, so we can support this. For example, if this module makes use of a Node.js API, then it still won't work in the web browser

So I guess it's not possible at the moment to use it in the browser. Am I missing something?

@wesleytodd
Copy link
Member

wesleytodd commented May 27, 2019

I think that comment is misleading for this discussion. Doug is pointing out that if we wanted to promote browser support we would need to add tests. And if this module did use a node api it would need something to support browsers. But this module does not have such a node api usage. If you import this into the browser today it should work just fine. I think I have even done so.

@BorntraegerMarc
Copy link

oh ok, so I completely misunderstood 😅 sorry for bothering then 😃

@wesleytodd
Copy link
Member

No worries, hopefully this conversation will help people in the future who land here :)

@markcellus
Copy link

markcellus commented Sep 21, 2019

FYI I've created an ES Module version of this package called cookie-esm which is working flawlessly for me!

@franciscop
Copy link

Hey @mkay581, thanks for cookie-esm! I'm working on sign, which is like cookie-signature but also supports browsers implementation (and Cloudflare Workers, etc), so it's a good combination and I'll use it in my current experiment.

Heads up, seems like your npm package is still 0.4.0 while the Github one is 1.0.1.

@markcellus
Copy link

Heads up, seems like your npm package is still 0.4.0 while the Github one is 1.0.1.

@franciscop that's the incorrect package haha. The cookie-esm package is at https://www.npmjs.com/package/cookie-esm. Latest version is 1.0.1.

@franciscop
Copy link

Oops, my bad! 👍

@ryhinchey
Copy link

@wesleytodd I think this issue can be safely closed. If developers want to use this library in an esm environment, they can. This works just fine:

import { parse, serialize } from 'cookie'

In addition, developers can use this module in the browser (provided they're using a bundler like rollup, browserify, webpack, etc).

@markcellus
Copy link

Sorry but this still isn't working for me. I'm using it in an esm environment in Chrome Browser via Karma using the @open-wc/karma-esm package and it's still not working. Out of all packages that are working with my setup, this package seems to be the only one that doesn't work.

I'll just have to keep using https://www.npmjs.com/package/cookie-esm package which has been a good workaround.

@ryhinchey
Copy link

@mkay581 You can use cookie in the brower with esm but you need to use a bundler/transpiler. Here's a working example using parcel via codesandbox. I'm not sure why it's not working with @open-wc/karma-esm. (maybe the fact that there's no "default" export?)

Given the versions of node this module must support and the fact that there's no transpile step for any modules under the jshttp org, I don't see this package working with esmodules in the browser out of the box any time soon

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

Successfully merging a pull request may close this issue.

7 participants