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

Converted to ES modules #135

Merged
merged 22 commits into from
Nov 9, 2023
Merged

Converted to ES modules #135

merged 22 commits into from
Nov 9, 2023

Conversation

stereobooster
Copy link
Contributor

Not sure if you are interested (if not feel free to close).

  • Changed all code to ES modules
  • Changed "bundler/builder" to microbundle (instead of Browserify and Babel)

As the result build size reduced from 100kb to 10kb

  • Added github CI
  • Updated all dependencies

Some speed improvements:

  • Changed {} to Object.create(null) (I guess, Map would be even faster, but I didn't want to change interface)
  • Use structuredClone instead for JSON.stringify/JSON.parse

PS recently wrote an article about front-end faceted search and apparently there are only 3 options

@cigolpl
Copy link
Member

cigolpl commented Nov 7, 2023

I've taken some time to read through the article you contributed, and I wanted to thank you for the thorough analysis.
While I haven't used TypeScript extensively before, I understand the benefits and I'm very pleased with your PR. I especially appreciate the significant reduction in the dist file size this will definitely benefit our end users. The updates to the libraries and the automated test system are working really well in my tests.

As we're moving forward, I've encountered a hiccup with our JSFiddle example. It's currently not operating as expected with the updated library.
I've updated the script source to use index.umd.js (https://6548b30bf4597f01be4be6ae--heartfelt-pithivier-087d23.netlify.app/index.umd.js) instead of itemsjs.min.js, but it's throwing a TypeError in the console:

fulltext.ts:23 Uncaught TypeError: o.default is not a function
    at new t (fulltext.ts:23:17)
    at index.ts:27:36
    at (index):188:11

The example is accessible here: https://jsfiddle.net/cigol/0ef9qeos/196/.
Could you have a look and advise on any modifications that might resolve this?

In addition, I'm proposing a small tweak for the clone function to maintain compatibility with Node.js 16, presented here:

export function clone<T>(val: T): T {
  try {
    // Try to use structuredClone if available.
    return structuredClone(val);
  } catch (e) {
    // If structuredClone is not available or fails, use JSON-based cloning as a fallback.
    return JSON.parse(JSON.stringify(val));
  }
}

Including this fallback in the PR would help us ensure a wider compatibility range.

Your input and contribution are very much valued, and I'm looking forward to any suggestions you might have.

@stereobooster
Copy link
Contributor Author

stereobooster commented Nov 7, 2023

While I haven't used TypeScript extensively before, I understand the benefits and I'm very pleased with your PR

To be sure we're on the same side. This PR doesn't use TypeScript. Maybe you checked out main branch of my fork. You want to check js branch instead.

I didn't pushed TS-version as PR, because thought it would be way too much changes at once

o.default is not a function

Microbundle can't resolve properly lunr package, which itself is published as UMD. That is tricky 🤔. I will need to check if there is a way around

In addition, I'm proposing a small tweak for the clone function to maintain compatibility with Node.js 16, presented here:

Sure

@stereobooster
Copy link
Contributor Author

stereobooster commented Nov 7, 2023

One way would be to convert lunr.js to ES modules as well, but it seems library haven't been updated in 3 years (olivernn/lunr.js#504). Even if I make a PR there, it probably won't be accepted

@stereobooster
Copy link
Contributor Author

There are 3 options (which I can think of):

  • fix issue in microbundle. I reported a bug, maybe they can help
  • refuse from UMD distribution and use ES modules in the browser. All modern browsers support it https://caniuse.com/?search=modules
  • Change lunr to something else

@cigolpl
Copy link
Member

cigolpl commented Nov 9, 2023

Yes, I was testing the TypeScript branch before (with vitest as testing environment)

Thank you for the suggestions on how to address the issue we're facing.

Fixing the issue in microbundle: This would be the ideal solution, as it directly addresses the root of the problem. However, this is also the option with the least predictability since it relies on external maintainers.

Refusing from UMD distribution to use ES modules in the browser: It's worth noting that while modern browsers support ES modules, a complete shift might not accommodate a significant segment of developers who continue to use UMD. Many libraries still support UMD, which suggests that it remains a considerable part of the JavaScript ecosystem. Without exact market data, it's challenging to quantify this, but it's clear that UMD usage is still extensive.

Switching to an alternative library: I propose Minisearch. It's is a lightweight library that supports ES modules and has come up multiple times in our issue tracker. It seems to offer the functionalities we need for our search capabilities.

@stereobooster
Copy link
Contributor Author

Kind people from microbundle helped me to understand issue. microbundle doesn't "pack" dependencies by default, which makes sense - if you install package via npm all those dependencies would be installed as well. So if I move dependencies (lunr etc.) microbundle will pack them in final js. But now size of final package is bigger

56K index.cjs
55K index.modern.js
56K index.module.js
56K index.umd.js

still smaller than it was, though

@cigolpl cigolpl merged commit 1ee9742 into itemsapi:master Nov 9, 2023
@cigolpl
Copy link
Member

cigolpl commented Nov 9, 2023

Excellent work! The size of the compressed file is now two times smaller than before, which is a fantastic improvement.

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.

2 participants