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

[build] polymer build does not handle .mjs files #736

Open
keanulee opened this issue Oct 15, 2018 · 17 comments
Open

[build] polymer build does not handle .mjs files #736

keanulee opened this issue Oct 15, 2018 · 17 comments
Labels

Comments

@keanulee
Copy link
Contributor

Unlike polymer serve, polymer build doesn't know to treat .mjs files as JavaScript files.

import {
  createStore,
  compose,
  applyMiddleware,
  combineReducers
} from 'redux/es/redux.mjs';
       ~~~~~~~~~~~~~~~~~~~~

file:///Users/keanulee/Code/Polymer/pwa-starter-kit/src/store.js(16,8) error [could-not-load] - Unable to load import: No parser for for file type mjs

Another example: Polymer/pwa-starter-kit#249

@aomarks
Copy link
Member

aomarks commented Oct 15, 2018

Why not just use .js files?

See also #671

@keanulee
Copy link
Contributor Author

This would be more applicable to 3p libraries, for example https://unpkg.com/redux@4.0.1/es/redux.mjs (reduxjs/redux#3143 from @TimvdLippe ).

@rjcorwin
Copy link
Contributor

I've opted for an awkward install step to fix the polymer builds in my project.

npm install && mv node_modules/redux/es/redux.mjs node_modules/redux/es/redux.js

@mathiasbynens
Copy link
Contributor

Patch: #792. Regardless of any one project’s decision to use *.mjs or *.js for its source files, tooling must support *.mjs simply because people are using it.

@bicknellr
Copy link
Member

IMO, #792 seems reasonable given the current "file extension implies media type" behavior of the analyzer. It might be nice to split this process into two steps at some point to help out with forward compatibility:

Rather than just

parsers = new Map<string, Parser<ParsedDocument>>([
  ['html', new HtmlParser()],
  ['js', new JavaScriptParser()],
  ['mjs', new JavaScriptParser()],
  ['css', new CssParser()],
  ['json', new JsonParser()],
]);

maybe doing something more like

defaultMediaType = new Map<string, string>([
  ['html', 'text/html'],
  ['css', 'text/css'],
  ['js', 'application/javascript'],
  ['mjs', 'application/javascript'],
  ['json', 'application/json'],
  ...userSuppliedDefaultMediaTypes
]);

parsers = new Map<string, Parser<ParsedDocument>>([
  ['text/html', new HtmlParser()],
  ['text/css', new CssParser()],
  ['application/javascript', new JavaScriptParser()],
  ['application/json', new JsonParser()],
]);

and allowing the user to supply a 'file to media type' map + defaults for extensions somewhere like this

{
  // This section is used above as `userSuppliedDefaultMediaTypes`:
  "extensionToDefaultMediaType": [
    ["es", "application/javascript"],
  ],

  "fileToMediaType": [
    ["./path/to/a-file-with-a.strange-extension", "text/html"],
  ],
}

so that the process for taking a file and finding an appropriate parser becomes something like

function getParserForFileAtPath(path) {
  if (fileToMediaType.has(path)) {
    return parser.get(fileToMediaType.get(path));
  }

  if (defaultMediaType.has(extensionForPath(path))) {
    return parser.get(defaultMediaType.has(extensionForPath(path));
  }

  throw new Error("Can't figure out how to load your thing.");
}

@web-padawan
Copy link
Contributor

Is there any agreement around .mjs extension set in stone anywhere so that we could refer to?
The statement like "because people are using it" does not provide enough information about whether people are doing right and isn't it too early to use that extension at this point.

Especially, webpack maintainer expressed the position regarding .mjs here: webpack/webpack#7482 (comment)

Note that .mjs and ESM in .js behaves different. .mjs tries to be as compatible as possible to node.js. This means no module field and no __esModule for .mjs files. Also note that .mjs is still experimental until node.js support has finalized.

@mathiasbynens
Copy link
Contributor

@bicknellr That seems like a good follow-on change! It’s similar to the early Node.js plans of expanding their module support to non-.mjs extensions. (Minor nit: the JavaScript MIME type is text/javascript, not application/javascript.)


@web-padawan I’m not sure what kind of data you’re asking for. Several people in this thread have pointed out issues with their builds failing because one of their dependencies uses .mjs for JavaScript modules, which is currently the only way to use modules in Node.js. How is that not enough?

The only “agreement” you need is that .mjs means JavaScript module. It already does in Node.js’s experimental modules implementation, in webpack, in V8’s d8, in Chrome (where we have specific extension → MIME type mappings to recognize .mjs as JavaScript in DevTools and in Chrome Extensions), and in Python’s SimpleHTTPServer. Code examples in the HTML spec uses .mjs to distinguish modules from classic scripts. Work is underway to register .mjs at the IETF level. Given the incoming issues (cfr. this very thread) and the strong precedent in other projects, I don’t see how waiting for anything else is helpful.

A one-line patch (#792) can fix this in polymer-tools. Let’s just make this work?

@bicknellr
Copy link
Member

If we split it up into the two phased 'extension → media type → parser' and make the 'extension → media type' step configurable, then we wouldn't need to take an opinion trying to predict Node.js' decision (i.e. merge #792) and could just let users that want mjs to be interpreted as JavaScript in their projects specify it in their config.


the JavaScript MIME type is text/javascript

Weird, I was looking at this page which claims it was obsoleted:
https://www.iana.org/assignments/media-types/media-types.xhtml (search for text/javascript)
But the HTML spec page definitely disagrees:
https://html.spec.whatwg.org/multipage/scripting.html#scriptingLanguages

@mathiasbynens
Copy link
Contributor

mathiasbynens commented Nov 28, 2018

trying to predict Node.js' decision

No need to predict, when there’s agreement on the “minimal kernel”, which states that:

  • import statements will only support files with an .mjs extension, and will import only ES modules, for now.
  • In a later phase, the intention is to move forward with format databases to map extensions and support multiple use cases.

The first point corresponds to #792. The second point corresponds to your proposal. I don’t think the former should be blocked on the latter.

Re: the MIME type, indeed. IANA doesn’t really match reality. The IETF draft I linked to will codify fix this particular issue at the IANA level. (In practice, the MIME type doesn’t really matter as long as it’s a JS MIME type, but hey.)

@mathiasbynens
Copy link
Contributor

To clarify my earlier comment, I’ll respond to this:

let users that want mjs to be interpreted as JavaScript in their projects specify it in their config

Considering that users reporting this issue have stated that the *.mjs file lives somewhere in their dependency tree rather than necessarily in their own code, this doesn’t seem like a sufficient solution by itself. .mjs always means it’s a JavaScript module.

Your proposal would be a great follow-up to enable more flexibility with other extensions, but .mjs should be supported by default, matching other tooling.

@bicknellr
Copy link
Member

let users that want mjs to be interpreted as JavaScript in their projects specify it in their config

Considering that users reporting this issue have stated that the *.mjs file lives somewhere in their dependency tree rather than necessarily in their own code, this doesn’t seem like a sufficient solution by itself. .mjs always means it’s a JavaScript module.

Could you go into more detail about why that isn't sufficient? I think that the mapping is used for all files that the analyzer finds, not just those within your project. (#792 is based on this assumption, right?)

@mathiasbynens
Copy link
Contributor

I’m arguing that forcing your users to add a mapping for .mjs → JavaScript whenever something in their dependency tree uses .mjs is an unnecessary burden. There is no situation in which one would not want that .mjs mapping, so IMHO it should be part of the defaults (like what #792 does). Requiring a configuration change just to fix this seems bad from a user perspective, and I’d personally consider it “insufficient” as a fix to #736. That’s what I meant — apologies for phrasing it confusingly.

I share your assumption that both the default mappings (which #792 extends) + the custom mappings (which your follow-on proposal would enable the user to extend) would affect everything the analyzer finds. 👍🏻

@mathiasbynens
Copy link
Contributor

I think this immediate issue can be closed now that #792 is merged.

@luwes
Copy link

luwes commented Feb 16, 2019

Is there a way so Polymer prioritizes .mjs files?

I have a package with a bundle in a subfolder

swiss-element/hooks.mjs => ESM
swiss-element/hooks.js => UMD

And Polymer is picking up the .js first with
import { useEffect } from 'swiss-element/hooks';

which results in an error because CJS modules are not supported.

This works however
import { useEffect } from 'swiss-element/hooks.mjs';

I know it's a small detail but my project uses suffix less syntax for npm packages...

@hashamhabib
Copy link

@luwes Did you find the solution for prioritize .mjs files?

@luwes
Copy link

luwes commented Feb 21, 2019

@hashamhabib no, I didn't find a real solution.

Only explicitly defining the extension.
import { useEffect } from 'swiss-element/hooks.mjs';

@stale
Copy link

stale bot commented Mar 3, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants