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

Target multiple plattforms (node, browser) using a bundler (Browserify) #53

Closed
wants to merge 6 commits into from

Conversation

marlon360
Copy link

Goal: Run this module in a browser

Approach: Using an npm package
Cons:

  • another dependency
  • maybe breaks the module
  • dependency has to be up-to-date

Better Approach: Use a bundler to build an independet file for the browser
Pros:

  • no changes for the node version
  • always up-to-date (popular bundlers are better maintained than other file-extension modules)

This PR contains:

  • added parceljs as a bundler
  • build browser js file to browser/index.js
  • create a global variable window["mime-types"] to access functions in the browser
  • added tests via karma for Firefox
  • use the same test file, but replaces the library file depending on the environment
  • added browser tests in Travis CI
  • added scripts in package.json for building browser version and running browser tests

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I've been down this road before and there was nothing but trouble having a built file checked in. We would accidentally publish new releases but no one remembered to update the bundled version. Is there a way in which the browser version can not be checked in at all and instead built automatically when "npm publish" is ran? This way no one ever forgets to keep them in sync and we don't have duplicate checked in code.

@dougwilson
Copy link
Contributor

Also ping @pfeigl if you can confirm including the bundle in the npm package like yhis works for your use case.

@dougwilson
Copy link
Contributor

Besides the above issue to look into, I marked the PR as "needs docs" because (a) we need user docs for how users will use this (or, at the very least, to update the blurb that this is a Node.js module to add what the browser support is and which browsers) and (b) need maintainer / contributor does now that there are a lot of scripts.

I locally pulled this down and ran npm run build-browser and the browser/index.js file on my machine is different that what is checked in, even though I didn't make any changes. Not sure why that would be, but maybe that is expected, I'm just not sure. Any docs that can help myself and anyone else actually maintain this module after merging will be very helpful, otherwise I'm not sure what the use will be in the long run.

I also see there is a new test-browser script as well, but running that just gives me the following error:

$ npm run test-browser

> mime-types@2.1.18 pretest-browser C:\GitHub\nodejs-mime-types
> npm run build-browser


> mime-types@2.1.18 build-browser C:\GitHub\nodejs-mime-types
> parcel build index.js --out-dir browser/ --global mime-types --no-source-maps

∞  Building...

∞  Building index.js...
∞  Building index.js...
∞  Building index.js...
∞  Building db.json...
∞  Building browser.js...
√  Built in 1.42s.

browser\index.js    117.83 KB    31ms

> mime-types@2.1.18 test-browser C:\GitHub\nodejs-mime-types
> node_modules/karma/bin/karma start

'node_modules' is not recognized as an internal or external command,
operable program or batch file.

npm ERR! Windows_NT 10.0.17134
npm ERR! argv "C:\\Program Files\\nodejs\\node.exe" "C:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js" "run" "test-browser"
npm ERR! node v6.11.1
npm ERR! npm  v3.10.10
npm ERR! code ELIFECYCLE
npm ERR! mime-types@2.1.18 test-browser: `node_modules/karma/bin/karma start`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the mime-types@2.1.18 test-browser script 'node_modules/karma/bin/karma start'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the mime-types package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     node_modules/karma/bin/karma start
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs mime-types
npm ERR! Or if that isn't available, you can get their info via:
npm ERR!     npm owner ls mime-types
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     C:\GitHub\nodejs-mime-types\npm-debug.log

Not sure what's up with that, but I may be just missing something. I ran npm install and, besides for a LOT of warnings being printed, completed OK, so not sure if there is something else I need to set up (any docs around this would be sweet 👍 ).

@marlon360
Copy link
Author

I locally pulled this down and ran npm run build-browser and the browser/index.js file on my machine is different that what is checked in, even though I didn't make any changes. Not sure why that would be, but maybe that is expected, I'm just not sure. Any docs that can help myself and anyone else actually maintain this module after merging will be very helpful, otherwise I'm not sure what the use will be in the long run.

Got the same problem and this is not a consistent way to build. I will replace parcel with browserify.

I also see there is a new test-browser script as well, but running that just gives me the following error:

I did not test it on a Windows machine, but test on Mac and Linux (Travis CI) run without problems.
I found this in the Karma docs:

Typing ./node_modules/karma/bin/karma start sucks and so you might find it useful to install karma-cli globally. You will need to do this if you want to run Karma on Windows from the command line.

Before installing the cli, try the following:
./node_modules/karma/bin/karma start or
cd ./node_modules/karma/bin && karma start
If this not work, you have to install the karma cli and run karma start in the project.

@marlon360
Copy link
Author

So I've been down this road before and there was nothing but trouble having a built file checked in. We would accidentally publish new releases but no one remembered to update the bundled version. Is there a way in which the browser version can not be checked in at all and instead built automatically when "npm publish" is ran? This way no one ever forgets to keep them in sync and we don't have duplicate checked in code.

When you test the code, it automatically builds the browser version and then tests it. So you can not forget to build it (because you alway test before a commit).
But I do not know how to prevent duplicate checked in code.

@dougwilson
Copy link
Contributor

But I do not know how to prevent duplicate checked in code.

Can you add the file to .gitignore?

Before installing the cli, try the following:
./node_modules/karma/bin/karma start or
cd ./node_modules/karma/bin && karma start
If this not work, you have to install the karma cli and run karma start in the project.

Thanks, I will try this today. If it works, can this be what npm run test-browser does?

Got the same problem and this is not a consistent way to build. I will replace parcel with browserify.

Gotcha.

@marlon360
Copy link
Author

We just need the browser version in the npm package, but not in the version control.

Can you add the file to .gitignore?

This is a good solution.

What do you think about deploying with Travis CI?
The CI could automatically test, build and then publish to npm. So nobody will forget to build the new browser version.

@dougwilson
Copy link
Contributor

How would that work? Wouldn't publish get stuck waiting for me to type in my 2FA code?

@marlon360 marlon360 changed the title Target multiple plattforms (node, browser) using a bundler (parcel) Target multiple plattforms (node, browser) using a bundler (Browserify) May 22, 2018
@marlon360
Copy link
Author

Here is a good instruction:
How to publish Node.js packages with Travis CI and packagecloud

With environment variables you can set your API_TOKEN (just visible for you).

@dougwilson
Copy link
Contributor

Right, but even with my token, running the npm publish will still prompt me to enter in my 2FA code from my phone to allow publishing a package.

@marlon360
Copy link
Author

Maybe this helps:

I had to switch my 2FA mode to auth-only to use auth tokens on travis-ci. At least, this allows me to publish packages with travis deployment tool.
Of course, being able to generate tokens as asked here without losing the default 2FA mode would be nice.

Support auth tokens that do not need 2fa

@dougwilson
Copy link
Contributor

Looks like there is no solution from that issue, just a request to add a feature?

@marlon360
Copy link
Author

Looks like you have to drop 2FA to publish with Travis CI.
Or you have to run a build locally and then publish.

@dougwilson
Copy link
Contributor

Yea, I turned on 2FA because there was at least one instance where a npm publish I did included my API token in the package, unbeknownst to me. It was in a config.gypi file that npm included automatically. You can find a good write up at https://github.com/ChALkeR/notes/blob/master/Do-not-underestimate-credentials-leaks.md

Because of these issues ChALkeR pushed to get npm to add 2FA and I enabled it the instant it was available so I wouldn't get hit by this again.

@dougwilson
Copy link
Contributor

Just release this as a fork of the module to npm. Perhaps mime-types-browser or such.

@dougwilson dougwilson closed this Nov 29, 2018
@hustcc
Copy link

hustcc commented Aug 28, 2019

Why another package? This module is just only do something about string process, remove the path dependence is the best way. #52

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

Successfully merging this pull request may close these issues.

None yet

3 participants