Parser loading about music-metadata-browser HOT 14 CLOSED

@Borewit Thanks! 💯

from music-metadata-browser.

Thanks for reviewing @icidasset.

music-metadata-browser is derived from the node.js oriented music-metadata, using node modules.

Using node modules for client side as well seems to become more common. However running modules directly in the browser is not a straightforward thing. Although browser have now support for ES6 modules, using a module bundler like webpack seems to be the most common practice.

I am a big fan of keeping things simple, so I perfectly understand you just want to simply add the dependency by injecting a single JavaScript file. Problem with prebundling is that all the dependencies music-metadata-browser is using can not be smartly combined with overlapping (NPM) dependencies of other dependecies you might have. A modular structure can be flattened into a single script, but not the other way around.

from music-metadata-browser.

I figured I'd ask because I barely have any other NPM dependencies in v2 (it's primarily written in Elm). Totally makes sense what you're saying though 😉👍

Any thoughts on my parser-loading question?
Might be useful to add some documentation about this.
I solved it as follows (not sure if this is the way to go about it).

import * as mm from "music-metadata-browser"
import mpeg from "music-metadata/lib/mpeg"

self.musicMetadata = mm
self.musicMetadata.parsers = { mpeg }
self.musicMetadata.parsingOptions = { loadParser }

function loadParser(parser) {
  return Promise.resolve(
    new self.musicMetadata.parsers[parser]
  )
}

// Then later:
// self.musicMetadata.parseFromTokenizer(..., ..., self.musicMetadata.parsingOptions)

from music-metadata-browser.

There are two 'levels' of lazy loading possible, which are :

1. Lazy loading the JavaScript module source code (getting it over the network)
2. Lazy initializing of parser code (the module is read by JavaScript parser)

Via the options you can register a custom loader for the parser.
I added this on WebAmp's request, but they decided not to use it after all, because the parsers are so small, it would not add any value.

I am not to happy with the way I implemented this, I think it would be nicer to override the module loader, returning the module, instead if the ITokenParser. That would eliminate the new in your code I guess.

I am very impressed you got a working this way, it was not directly the intend of this option.

I am using require to lazy load the parser module, I will replace it with import, maybe that will be more browser friendly and solve your problem.

from music-metadata-browser.

Prevent using require in: v0.6.6.
Can you try that one @icidasset without setting option.loadParser?

from music-metadata-browser.

Same thing @Borewit

It's not entirely clear to me what is supposed to happen.
The v0.6.6 change is still lazy loading, right?
And in my scenario I would need to disable lazy loading.
So that when I bundle my javascript, the parsers are included already.

Did you have a different view on this?
You think I should be doing lazy loading instead?

I'm using Parcel as the bundler and I am building with this command:
parcel build index.js --no-source-maps --no-cache --no-minify

And with the following code:

import * as musicMetadata from "music-metadata-browser"
import { StreamingHttpTokenReader } from "streaming-http-token-reader"

self.musicMetadata = musicMetadata
self.StreamingHttpTokenReader = StreamingHttpTokenReader

Which I then test in the browser using:

const url = "OMITTED"
const reader = new StreamingHttpTokenReader(url)

reader
  .init()
  .then(() => { return musicMetadata.parseFromTokenizer(reader, reader.contentType) })
  .then(console.log)

PS. I don't mind my current solution.
Just so you know 😉

from music-metadata-browser.

It is still lazy loading, I thought maybe the browser has an issue with require. So I replaced it with import. But I just noticed it did not change anyhting in the JavaScript code. Maybe I have to change the module type.

I think the lazy loading causes Parcel to fail to detected the code required for the parsers.

As an experiment, maybe you can enforce the dependecies with some dummy code:

import mpeg from "music-metadata/lib/mpeg/index"
import flac from "music-metadata/lib/flac"

// Same for: 'mpeg' | 'apev2' | 'mp4' | 'asf' | 'flac' | 'ogg' | 'aiff' | 'wavpack' | 'riff' | 'musepack'
const parsers = [mpeg, flac];

I think Parcel should be able to support the lazy loading: https://parceljs.org/code_splitting.html

The await import is exactly what I am using on the TypeScript level.

Can I check out your code?
I noticed your v2 branch, but that one seems a bit older.

from music-metadata-browser.

ES6 module loading is experimental in Node, maybe I should override the loader in music-metadata-browser with ES6.

Can you try to use ES6 dynamic loading in your code, maybe in comination with guidance of Parcel? You need a function simular to this (which more or less the same as the original TypeScript code in music-metadata)

// parser string like: 'mpeg' | 'apev2' | 'mp4' | 'asf' | 'flac' | 'ogg' | 'aiff' | 'wavpack' | 'riff' | 'musepack'
function async loadParser(parser) {
  const module = await import('music-metadata/lib/' + parser); // or parser + '/index'
  return new module.default();
}

from music-metadata-browser.

Experiment 1 = failed (imports and const parsers)
Experiment 2 = success

The dynamic loading works if I manually copy all the parser files from inside node_modules to my dist output (aka. build) directory. Which is expected 👍as described in the Parcel docs (ie. each parser is treated as a separate js bundle).

I think I'm going to stick with my original solution, as I prefer non-lazy loading. It doesn't look like Parcel has the ability to transform lazy-loading imports to strict imports.

And yeah, this javascript code isn't in diffuse, I'm trying to keep heavy NPM dependencies out of the project. I have a separate git repo for this, nothing in it that I haven't shown you though.

Thanks for all the help! ☺️
Feel free to close this if you want to.

from music-metadata-browser.

Hello,

I'm having a similar error as icidasset. I'm trying to get metadata of mp3 files from a webapp. I'm using browserify to make the "require"s work and compile it with

$ ./node_modules/.bin/browserify www/test.js -o www/testb.js -r music-metadata-browser

On the phone I get this error: «Uncaught (in promise) Error: Cannot find module './mpeg/index'» this is the code:

window.mm = require("music-metadata-browser");
mm.fetchFromUrl("http://192.168.1.10:8000/Balanced.mp3").then(m => console.log(m))

I read several times all the messages but I didn't get how was the issue fixed.

Thanks in advance

from music-metadata-browser.

Thanks for letting me know @alexbcberio.

I think I will eliminate the lazy loading of parsers. The parsers are not that big. I prefer music-metadatabrowser is working without too much of a hassle.

As Donald Knuth stated "premature optimization is the root of all evil".

from music-metadata-browser.

Lazy loading disabled in v0.7.0.

If you can let me know if things a working better for you that would be great.

from music-metadata-browser.

Thanks @Borewit for the fast response, updating to v0.7.0 made it work with no changes in code.

Just you to know, the response.body.cancel() call on fetchFromUrl() function is throwing the next error:

Uncaught (in promise) TypeError: Cannot cancel a readable stream that is locked to a reader
    at parseReadableStream.then.res

The metadata is returned correctly in spite of the error

from music-metadata-browser.

Good to know it is working @alexbcberio.

I have seen that error, on the cancel, before.
If it is bothering you, please report it in a new issue.

from music-metadata-browser.