Polymer / tools

Polymer Tools Monorepo
BSD 3-Clause "New" or "Revised" License
430 stars 199 forks source link

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

Open keanulee opened 5 years ago

keanulee commented 5 years ago

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: https://github.com/Polymer/pwa-starter-kit/pull/249

aomarks commented 5 years ago

Why not just use .js files?

See also https://github.com/Polymer/tools/issues/671

keanulee commented 5 years ago

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

rjcorwin commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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: https://github.com/webpack/webpack/issues/7482#issuecomment-394838925

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

luwes commented 5 years ago

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 commented 5 years ago

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

luwes commented 5 years ago

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

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

stale[bot] commented 4 years ago

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.