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

Adding ESM support. #233

Closed
wants to merge 1 commit into from
Closed

Conversation

janesser
Copy link

@janesser janesser commented Apr 9, 2020

If you have an issue with a specific extension or type

Locate the definition for your extension/type in the db.json file in the mime-db project. Does it look right?

@janesser janesser force-pushed the feature/rollup-esm branch from 5706cc1 to 6460b7c Compare April 9, 2020 13:10
@janesser
Copy link
Author

@broofa any concerns about merging this one?

Copy link
Owner

@broofa broofa left a comment

Choose a reason for hiding this comment

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

Some comments follow but, honestly, I'm going to suggest that if Deno needs a mime module, you/they might want to work with the JSHttp team that maintains the mime-types module. It has been my intention for awhile now to hand this package off to that org.

FWIW, the reason I'm dragging my heels on this is that I'm not familiar with Deno. If I take this PR, I don't have a good solution for how to maintain it. E.g. if it breaks for other Deno users and they complain, how do those issues get handled?

Is there a way to make this module "deno ready" without having to use a packager like rollup?

README.md Outdated
@@ -43,6 +43,14 @@ Or, for the `mime/lite` version:
mimelite.getType(...); // (Note `mimelite` here)
<script>

## Deno
Despite its name this module can be used not just in node context, but also with [deno](https://deno.land), which is kind of the successor of node.js.
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: Avoid "passive voice". State things confidently or delete 'em. :-)

Also, the module's name is mime. It's only the repo that's called node-mime... and I should probably fix that.

Suggested change
Despite its name this module can be used not just in node context, but also with [deno](https://deno.land), which is kind of the successor of node.js.
This module can be used with [deno](https://deno.land).

README.md Outdated
## Deno
Despite its name this module can be used not just in node context, but also with [deno](https://deno.land), which is kind of the successor of node.js.

import Mime from "../node-mime/dist/mime.esm.js";
Copy link
Owner

Choose a reason for hiding this comment

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

This import path seems... weird. What directory is it relative to? How were these files installed? In JS/NPM land, these files install into node_modules/mime, so I would not expect to see "node-mime" as part of the import path.

This is where my lack of Deno experience raises a lot of questions about how Deno users would normally expect to approach this module.

Copy link
Author

@janesser janesser Apr 17, 2020

Choose a reason for hiding this comment

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

deno would actually refer to modules by URL, something with raw.github...
to hell with the node_modules folder ;-)

ill fix it.

@janesser
Copy link
Author

@broofa in the end it's about providing a es6-module-bundle on some URL, nothing more, nothing less. I would still be okay, to not mention deno as it might make allusion to any support neither you nor me are willing to give.

Also looke HERE where i used that branch already. It was quite straight forward.
https://github.com/NMathar/deno-express/pull/4/files

@janesser
Copy link
Author

janesser commented May 3, 2020

@broofa what's the next move here?

@janesser janesser force-pushed the feature/rollup-esm branch from 12c0151 to 1315019 Compare May 3, 2020 09:52
Copy link
Owner

@broofa broofa left a comment

Choose a reason for hiding this comment

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

A few detailed comments, below, which you're welcome to make.

"However ..."

First, thank you for your time and effort here. I do appreciate it, even if I'm not exactly prompt in my responses.

Correct me if I'm wrong, but what we're really talking about here is the more general feature of adding ESM support. If we do that, then we get Deno support more or less for free, right?

If that's the case, then I'd like to change the scope of this work accordingly. I know that's not what you want, but this will be of much more value to existing/future users of this module.

If you want to take a stab at that, that would be great. If not, I'll try to bang something out in the next couple of weeks. (No promises... in case it's not obvious, my time/attention for maintenance here is a bit limited.).

README.md Outdated
@@ -43,6 +43,14 @@ Or, for the `mime/lite` version:
mimelite.getType(...); // (Note `mimelite` here)
<script>

## Deno
Deno, the successor project of the maker node.js, is based on es6-modules, imported by URL, and thereby obsoleting the node_modules folder and the related module lookup mechanism.
Copy link
Owner

Choose a reason for hiding this comment

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

No need to expound on what Deno is. Readers will either know or they won't.

Suggested change
Deno, the successor project of the maker node.js, is based on es6-modules, imported by URL, and thereby obsoleting the node_modules folder and the related module lookup mechanism.
Deno users may install as follows:

README.md Outdated
## Deno
Deno, the successor project of the maker node.js, is based on es6-modules, imported by URL, and thereby obsoleting the node_modules folder and the related module lookup mechanism.

import Mime from "https://raw.githubusercontent.com/broofa/node-mime/master/dist/mime.esm.js";
Copy link
Owner

Choose a reason for hiding this comment

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

Linking to HEAD on master bypasses the formal release process I use (such as it is). Also, master is expected to break occasionally. I can't in good conscience tell people to do this.

README.md Outdated
@@ -43,6 +43,14 @@ Or, for the `mime/lite` version:
mimelite.getType(...); // (Note `mimelite` here)
<script>

## Deno
Copy link
Owner

Choose a reason for hiding this comment

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

From line 2 of this file:

-- This file is auto-generated from src/README_js.md. Changes should be made there.

Also, there should probably be instructions for importing the "lite" version module as well.

README.md Outdated
import Mime from "https://raw.githubusercontent.com/broofa/node-mime/master/dist/mime.esm.js";
Mime.getType(...)

The contents behind the URL will be cached locally, until deno is explicitly told to refresh them.
Copy link
Owner

Choose a reason for hiding this comment

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

I'm skeptical this is of much use in modern release pipelines where you have CI, staged builds, cloud deployment, etc.

Suggested change
The contents behind the URL will be cached locally, until deno is explicitly told to refresh them.

@janesser janesser changed the title Build an esm with rollup which is compatible to deno. Adding ESM support. May 4, 2020
@janesser janesser force-pushed the feature/rollup-esm branch 2 times, most recently from 38000d8 to a171d4b Compare May 10, 2020 10:09
@janesser
Copy link
Author

@broofa i spent some time, refining the PR in the way we discussed.

As a result the type of present module is now explicitly declared as '"type": "commonjs"' (was like it by default).
Thereby adopting merely the option presented here: https://2ality.com/2019/10/hybrid-npm-packages.html#support-for-esm, declaring '"module": "..."' as an extra.

The one thing left open to me is,which is gonna be the resulting URL after the releasing process.
Can you help me predict it? It would then be documented as pattern for browser usage (which again is much similar to deno-import). Updating the README_js.md in this regards would be last TODO to handle.

@lukeed
Copy link

lukeed commented Aug 4, 2021

Just to be clear – since some may be seeing this as a PR for adding native ESM support – this is only adding an ESM file, which could be used by bundlers that respect the module entry field. Even for this target usecase, this PR is missing the lite.mjs variant.

In order to add actual native ESM support, a number of other changes would have to be made. Doing so should warrant a 3.0 release since the addition of an exports map can be a breaking change for Node 13.0 - 13.7 users (please upgrade if this is you). At this point, I (personally) consider adjusting the minimum engines support and/or publishing to Deno.

@janesser janesser force-pushed the feature/rollup-esm branch from e9eaf65 to 00bae82 Compare August 17, 2021 15:22
@janesser
Copy link
Author

@lukeed i rebased the PR, and added lite.mjs as suggested.
rollup seems to have dropped node8 support, so that CI will not fully pass.

Regarding the releases, please refer to @broofa

Cheers, Jan

@broofa
Copy link
Owner

broofa commented Sep 19, 2023

Hey Jan, my apologies for letting this PR languish like this. I'm not even going to pretend there's a good excuse for it. Just... again, my apologies.

I'm going to close this out now that I've released mime@4 (npm install mime@beta`). It has full ESM support, as well as a few other goodies. I know asking you to take a look at provide feedback is pretty presumptuous, considering how I've ignored your contribution here, but I promise I'll be better about this in the future.

Cheers!

@broofa broofa closed this Sep 19, 2023
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