okikio / bundlejs

An online tool to quickly bundle & minify your projects, while viewing the compressed gzip/brotli bundle size, all running locally on your browser.
https://bundlejs.com
MIT License
751 stars 13 forks source link

API error #50

Closed hatemhosny closed 1 year ago

hatemhosny commented 1 year ago

hi @okikio ,

great work 👍

When using the API, I'm getting the same response for any import URL

e.g. https://deno.bundlejs.com/?file&q=https://cdn.jsdelivr.net/gh/remeda/remeda@master/src/flatten.ts https://deno.bundlejs.com/?file&q=https://cdn.jsdelivr.net/gh/remeda/remeda@master/src/concat.ts https://deno.bundlejs.com/?file&q=https://cdn.jsdelivr.net/gh/remeda/remeda@master/src/not-existing.ts https://deno.bundlejs.com/?file&q=https://cdn.jsdelivr.net/not-existing.ts

which seems to be the library "spring-easing"

is this a cache issue?

Thank you very much

okikio commented 1 year ago

Oh, sorry this seems like a bug, I'll get right on it

okikio commented 1 year ago

@hatemhosny This should now be fixed

hatemhosny commented 1 year ago

Thank you @okikio

Named exports are not correct

https://deno.bundlejs.com/?file&q=https://cdn.jsdelivr.net/gh/remeda/remeda@master/src/flatten.ts

exports this

export{f as httpsCdnjsdelivrnetGhRemedaRemedamasterSrcFlattents};

it should be

export{f as flatten};

Much appreciated 🙏

okikio commented 1 year ago

Ok, I'll look into this

okikio commented 1 year ago

@hatemhosny Done, the API should now format exports properly

hatemhosny commented 1 year ago

Thank you, @okikio for your help. I really appreciate the effort you put into this amazing project.

I still think something is not correct.

please consider this source code: https://cdn.jsdelivr.net/gh/remeda/remeda@master/src/concat.ts

this code should work:

import { concat } from 'https://deno.bundlejs.com/?file&q=https://cdn.jsdelivr.net/gh/remeda/remeda@master/src/concat.ts';
console.log(concat(['a'])([1, 2, 3]));  // Uncaught TypeError: concat is not a function

however, exports seem to be wrapped inside extra object.
so I need to do this for it to work:

import { concat } from 'https://deno.bundlejs.com/?file&q=https://cdn.jsdelivr.net/gh/remeda/remeda@master/src/concat.ts';
const c = concat.concat //  <--- here
console.log(c(['a'])([1, 2, 3]));  // [1, 2, 3, 'a']

in addition, https://deno.bundlejs.com/?file&q=https://cdn.jsdelivr.net/gh/remeda/remeda@master/src/flatten.ts still gives "spring-easing"
I guess this might be a caching issue that would resolve with time (I'm not sure)

Thank you and sorry for overloading

hatemhosny commented 1 year ago

@okikio

I want to mention that object wrapping problem did not occur previously, then started recently (not sure when).
just in case that guides finding the problem.

related to this, would you consider providing versioned API endpoints, so that each version would have a consistent behavior and avoid breaking changes on updates? just a suggestion.

okikio commented 1 year ago

@hatemhosny Yeah, for sure, I'm going to do that but I need to do that for everything, I'm planning a rewrite of bundlejs such that the versions better match but that would require that both the share url of the website and API match, you can check out the beta version at https://bundlejs.netlify.app, it's faster leaner and just better, but it's still a WIP

The goal is to have a bundlejs library, api and website that all share the same version, still WIP of course but getting close and closer everyday

hatemhosny commented 1 year ago

@okikio

So I can see the bundle ends by export{<token> as <filename>}; (e.g. export{u as flatten};)

the correct form would probably be export <token>; (e.g. export u;)

it works if I do this:

?file&q=<url>&treeshake=[{<filename>}]
e.g.: https://deno.bundlejs.com/?file&q=https://cdn.jsdelivr.net/gh/remeda/remeda@master/src/flatten.ts&treeshake=[{flatten}]

I'm not sure if that helps

okikio commented 1 year ago

@hatemhosny Yeah, I haven't had time to fix it yet but it should be an easy fix

hatemhosny commented 1 year ago

@okikio

So I can see the bundle ends by export{<token> as <filename>}; (e.g. export{u as flatten};)

the correct form would probably be export <token>; (e.g. export u;)

it works if I do this:

?file&q=<url>&treeshake=[{<filename>}] e.g.: https://deno.bundlejs.com/?file&q=https://cdn.jsdelivr.net/gh/remeda/remeda@master/src/flatten.ts&treeshake=[{flatten}]

I'm not sure if that helps

Hi @okikio ,

do you think I can help fixing this issue?

I'm not familiar with the codebase, but if you guide me I can try to send a PR with a fix when I have time.

Meanwhile, do you think it is suitable to re-open this issue, so that it is kept in the plans?

thank you

okikio commented 1 year ago

Thank you so much for bringing it to my attention I forgot about it for a little bit there, I'll get it fixed. Also, I'm working on drastically improving the docs so it's easier to contribute to the project

okikio commented 1 year ago

~Looking further at the problem I don't think this is a bug, from my understanding this is expected behaviour, I think the behaviour you're referring to is when it comes to~

~https://deno.bundlejs.com/?file&q=https://cdn.jsdelivr.net/gh/remeda/remeda@master/src/flatten.ts&treeshake=[*]~

~Unless I'm misunderstanding the goal, because by default we only use the filename in exports when the user is exporting multiple files with default exports, it's not automatic and not guranteed, we only give an export a name if there are possible conflicts with other exported modules. Do you think we should change this behaviour?~

Wait, that was an error on my part I see what you mean now https://github.com/okikio/bundlejs/issues/50#issuecomment-1553959545

okikio commented 1 year ago

Ok, so yes there was an error with the way bundlejs worked, the issue stems from how to handle CommonJS files.

"CommonJS file only export their default, that made it complex to work with, since how would I know a module is CommonJS if I don't fetch it, but if I do fetch it, it would be too late for esbuild to deal with it properly"

However, I think I now have a workable solution that doesn't matter if the module is ESM or CommonJS

  1. For a single CJS file, only export the default (don't rename anything) https://deno.bundlejs.com/no-cache?file&q=postcss-easings&minify=false
  2. For multiple CJS files export the default but use the file url as the import name, and make sure to add ...Default e.g. reactDefault & reactDomDefault https://deno.bundlejs.com/no-cache?file&q=react,react-dom&minify=false
  3. For single ESM files export all including the default (don't rename anything)
  4. For multiple ESM files export all methods and variables from all exports, however, use the naming rules for all default exports, e.g. so after exporting the other module exports, rename the default export to springEasingDefault and codepointIteratorDefault https://deno.bundlejs.com/no-cache?file&q=spring-easing,codepoint-iterator&minify=false
  5. Treeshaking: We will assume you want complete control if you add the treeshake query param to the URL, so all the above rules become null and void, you now get absolute control over what is exported, meaning default exports won't be automatic

    Note: Meaning you can run into issues with CJS modules if you don't export default properly. Part of it is because treeshaking doesn't really exist or even work for CJS packages, keep this in mind

okikio commented 1 year ago

I've documented the new changes in a blog post https://blog.okikio.dev/mastering-the-art-of-esm-and-cjs-package-handling

hatemhosny commented 1 year ago

Great! Thank you.

I confirm it works now as expected.

A great project. Thanks a lot.

I'm using the API in LiveCodes playground to allow importing deno modules and modules from GitHub (by converting github urls to that of jsdeliver) as documented here and here. This would allow the user to run code like this:

import { flatten } from 'https://github.com/remeda/remeda/blob/master/src/flatten.ts';
console.log(flatten([[1, 2], [3], [4, 5]])); // -> [1, 2, 3, 4, 5]

I have added credit to bundlejs in the credits page and in the repo README.

Thank you :)

okikio commented 1 year ago

Thanks 😊 love to see others finding value in bundlejs

hatemhosny commented 1 year ago

Hi @okikio

Now https://deno.bundlejs.com/?file&q=https://cdn.jsdelivr.net/gh/remeda/remeda@master/src/flatten.ts bundles react and react-dom

okikio commented 1 year ago

That must be a weird cache mix up 😅, I literally just pushed a change today to fix bugs and I thought introduced a new bug for a sec there, but the issue is that i did not yet clear the cache.

After I cleared the full bundlejs cache it seems to be working properly again.

Please try it again to confirm.

They're not kidding when they say caching is a tough computer science problem

hatemhosny commented 1 year ago

It does work now. Thank you :)

hatemhosny commented 1 year ago

@okikio by the way, I have publicly released LiveCodes. Please read the announcement post.

These are the official documentations which describe importing deno modules and modules from GitHub and GitLab in the playground. This functionality is provided by bundlejs API. Thank you for making that possible.

As mentioned before, the credit to bundlejs is added in the repo readme and in the website credits page.

❤️

okikio commented 1 year ago

Congrats. Thanks 😊