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

Make project multiplatform #91

Merged
merged 19 commits into from
Jul 6, 2022
Merged

Conversation

BobVarioa
Copy link
Collaborator

Description

This disconnects the project from node, and makes the project multi platform. This adds esbuild as a build system and separates the test system into platform specific pieces. This also adds a few dependencies so that might be something you want to avoid.

Type of change

  • New feature (non-breaking change which adds functionality)
  • [?] This change requires a documentation update (it now works on browser, so potentially)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • [?] I have made corresponding changes to the documentation (see above)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have checked my code and corrected any misspellings

@LuanRT
Copy link
Owner

LuanRT commented Jul 5, 2022

This looks great! I'll review the changes and do some testing. Meanwhile, could you fix the conflicts?

Thank you.

@LuanRT
Copy link
Owner

LuanRT commented Jul 6, 2022

Ok, so I'm back —

Everything is running pretty nicely and I like the idea of browser support, but I noticed a few things:

  1. ESbuild strips away all jsdoc comments from the code, rendering them useless (eg; no proper code completion, no variable types).

    Not really sure how this could be fixed, maybe bundle only for the browser and leave node as it is?

  2. Class names are a bit messed up, ex:

    const music_search = await session.music.search('Mac Miller, thoughts from a balcony', { type: 'song' });

    Returns:

     Search2 {
       //...
     }

    I'm not an expert in esbuild but maybe --keep-names would fix that?

  3. Type declarations are broken because the entry file is pointing to the bundled code instead of the main code base (related to 1). And the idb package seems to be riddled with typescript errors (or maybe I'm missing something)? Which prevents type declarations from being generated altogether.

@BobVarioa
Copy link
Collaborator Author

BobVarioa commented Jul 6, 2022

So, unfortunately bundling is a tough problem, and we can't really use --keep-names because that just assures that the legacy variable name is the same throughout minification which doesn't really help us in this case. The best solution I could come up with is to include source maps which will map the minified code to the actual source, and in environments like the browser it will automatically look the same (node requires an additional flag --enable-source-maps, but also has the same behavior).
Types should work after this change, unless you use explicit imports of build/node.js, build/browser.js, etc. which sadly for us because our tests do not use a bundler, means that types will be unavailable there, but to any end user it should look normal. Just an aside, the issue with idb was just a missing lib.dom.d.ts because typescript thought it was just a node environment.

@LuanRT
Copy link
Owner

LuanRT commented Jul 6, 2022

Great, looks pretty solid now. This is a huge improvement to the library so thank you! Also, for that, I am inviting you as a collaborator to the project – if you would like to join just check your email and accept the invite.

@LuanRT LuanRT merged commit f52d15c into LuanRT:main Jul 6, 2022
@Wykerd Wykerd mentioned this pull request Jul 11, 2022
13 tasks
@Wykerd Wykerd added this to the v2 milestone Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants