catamphetamine / libphonenumber-js

A simpler (and smaller) rewrite of Google Android's libphonenumber library in javascript
https://catamphetamine.gitlab.io/libphonenumber-js/
MIT License
2.79k stars 216 forks source link

Loading JSON file with ES6 import to browser #239

Closed madiyetov closed 5 years ago

madiyetov commented 6 years ago

We use ES6 import without any build step. ES6 import doesn't load json files due to HTML spec. So I am getting next error when loading index.es6.js: Failed to load module script: The server responded with a non-JavaScript MIME type of "application/json". Strict MIME type checking is enforced for module scripts per HTML spec.

So proper way will be to load javascript file which will export js object: https://stackoverflow.com/questions/34944099/how-to-import-a-json-file-in-ecmascript-6

catamphetamine commented 6 years ago

Not understand

catamphetamine commented 6 years ago

Seems that .js extension is also required, not just .json. I won't be changing all imports to .js extension.

RoXuS commented 5 years ago

+1 on browser it doesn't work due to import json...

catamphetamine commented 5 years ago

Ok, I'm reopening this if people're having issues with it. But I won't be changing .json to .js.

RoXuS commented 5 years ago

Browser don't support direct json import so you have to transform the json to an export in js file.

brianreeve commented 5 years ago

+1 or ES6 imports of the lib won't work natively in browsers. Your own README examples are failing in modern browsers. For example:

import { AsYouType } from 'libphonenumber-js'

IMO your hard stance on this is fighting an unwinnable battle: "Strict MIME type checking is enforced for module scripts per HTML spec." Solution proposed by OP is the way to go.

catamphetamine commented 5 years ago

@brianreeve If your proposed solution is adding .js extensions to all imports then the answer is "No".

catamphetamine commented 5 years ago

Actually, I would add .js extensions to all imports if the spec also supported importing .json. If the spec doesn't know how to import .json files then that's a bad spec and it should be fixed. To conclude, the .json files are left .json unless extremely required to be .js.

catamphetamine commented 5 years ago

Well, you could test if the non-JSON version works first. There're several sub-packages and core is one of them. You could somehow obtain metadata.json as a variable in the app and then import the core subpackage and pass metadata as the last argument. If that works then we could work from there. Otherwise post the error.

catamphetamine commented 5 years ago

As a last resort, the library could ship .json.js files just for the inferior ES6 importing spec that could be used with the /custom subpackage.

catamphetamine commented 5 years ago

So do the testing and I'll do the rest.

brianreeve commented 5 years ago

I'm very surprised at the resistance to using the platform and moving towards removing the dependency and complexity of build tools. I'm not attempting to engage in debate or argument. I was just adding to what others were saying above. As-is, this library is unusable on the modern web using imports natively.

Yes, webpack supports importing json directly through the json loader. However, it is not based on any approved standard. The first sentence of the MDN docs plainly says import "..is used to import features into JavaScript modules that have been exported by another module." A JSON file is not a module. Additionally, under the covers, webpack's json-loader converts a json file to a proper export.

Since ES6 modules are so widely supported now, I don't plan to have any build step required for the web components I am developing.

As a quick test, I made a couple changes:

  1. Saved metadata.full.json as metadata.full.js and modified it to be:

export default {"version":"1.7.16","country_calling_codes"...

  1. Updated index.js to change:

import metadata from '../metadata.full.json'

to

import metadata from '../metadata.full.js'

There are some downstream issues with loading the semver-compare dependency that I haven't yet looked into, but the above seems to get past the bug with importing json metadata files.

Would it be acceptable to build js metadata files which export the metadata object, and change the imports to use the js file instead of the JSON?

catamphetamine commented 5 years ago

I'm very surprised at the resistance to using the platform and moving towards removing the dependency and complexity of build tools.

I support the general initiative, not the actual implementation.

I'm not attempting to engage in debate or argument. I was just adding to what others were saying above.

Debating is fun and should be encouraged.

As-is, this library is unusable on the modern web using imports natively.

Yes, as-is it is unusable on the inferior "modern" import platform. When they fix their platform then we'll talk.

Yes, webpack supports importing json directly through the json loader. However, it is not based on any approved standard.

JSON is an extremely convenient and standardized data format across the whole world and so it would make perfect sense for the spec authors to add the ability to import json. Not doing so is oppressing the minority that favours json over js. Isn't Western civilization all about protecting the minorities of all kinds? If yes, then we must fight for our right to keep json.

Since ES6 modules are so widely supported now, I don't plan to have any build step required for the web components I am developing.

But you can still volunteer into testing, otherwise you'd be contradicting yourself being an evangelist of the platform.

There are some downstream issues with loading the semver-compare dependency that I haven't yet looked into

Yeah, that's the point: no one supports that new importing system of yours. semver-compare could very easily be inlined though removing the dependency: https://github.com/substack/semver-compare/blob/master/index.js

but the above seems to get past the bug with importing json metadata files.

So we don't need to change all imports to .js then. The only thing not supported by the import spec is the .json import then.

The conclusion follows that this library is able to get working with the import platform. The workaround steps are:

metadata can be obtained by importing the .json.js files which can be generated from .json files by prepending export default to them. I could even see it being part of this library's distribution package.

There is a comment though in the discussion: https://stackoverflow.com/questions/34944099/how-to-import-a-json-file-in-ecmascript-6

ES6 support JSON importing with the following syntax: import * as data from './example.json'

Someone should test that.

brianreeve commented 5 years ago

Debating is fun and should be encouraged.

Thanks for your additional comments. I just meant that I wasn't trying to be contrary or difficult in my challenges to your replies.

JSON is an extremely convenient and standardized data format across the whole world and so it would make perfect sense for the spec authors to add the ability to import json.

Not doing so is oppressing the minority that favours json over js.

I can appreciate your sentiment, but don't agree with your assessment that minority opinions are being oppressed if they don't make it into a spec. I don't even think that being a fan of json puts anyone in a minority (I definitely like it), but I imagine spec authors must consider A LOT of external and internal factors when determining what makes it in.

More to the point, as written, the import spec and implementation is clearly not intended for anything other than javascript modules. Calling it "inferior" to webpack's json-loader approach is not constructive. I would argue webpack's importing of json is a misuse of the feature.

Depending on the use case, there are alternative solutions that are fairly simple, such as loading JSON data via XHR, or exporting an object as we've seen.

If the spec allowed importing non-js files, like json, then why stop there? Why not allow importing text files like csv, yml, or binaries like images, videos, etc.? As you can see it would quickly spiral into deeper implications with security, memory management, caching, etc. since there are already appropriate mechanisms in place to manage those.

Yeah, that's the point: no one supports that new importing system of yours.

This is a bold statement. It has been in the spec since ECMAScript 2015 and is now very widely supported across browsers. Libraries are definitely shipping with using import.

The fact is the platform technology has properly plugged the holes which were previously patched through tooling and process. Since authors of the tooling are not required to follow any standard (or wrote the patchwork prior to specs being finalized), we're now in a period where existing packages need to be adjusted to reconcile the differences and run as expected natively.

IMO, to resist that change is to resist forward movement.

metadata can be obtained by importing the .json.js files which can be generated from .json files by prepending export default to them. I could even see it being part of this library's distribution package.

I don't fully understand your proposed workaround you gave in your reply, but the above statement is in agreement with the last sentence of my previous post 💯.

ES6 support JSON importing with the following syntax: import * as data from './example.json'

BTW, I did try this earlier and it fails with the same error as originally reported.

Regarding semver-compare, it does seem that it would be reasonable to inline or absorb into your project since it's a single function. Just out of curiosity, is it necessary any more? It seems that you are only shipping one version of metadata, but the code supports 3 separate versions of the json?

I'm happy to test any changes you're planning on implementing to support native module imports. I'm following this thread, so I'll try to be responsive to test any updates.

Thanks.

catamphetamine commented 5 years ago

@brianreeve Well, ok, I'll play along with their spec for now. Released libphonenumber-js@1.7.18, see if it works. I still consider the spec flawed.

brianreeve commented 5 years ago

Great! I updated the package and now have no issue importing AsYouType like:

import { AsYouType } from 'libphonenumber-js/max'

Thanks!

mikea commented 5 years ago

The fix for this issue breaks TypeScript json imports:

error TS7016: Could not find a declaration file for module 'libphonenumber-js/examples.mobile.json'. '/node_modules/libphonenumber-js/examples.mobile.json.js' implicitly has an 'any' type. Try npm install @types/libphonenumber-js if it exists or add a new declaration (.d.ts) file containing declare module 'libphonenumber-js/examples.mobile.json';

catamphetamine commented 5 years ago

@mikea TypeScript support for this package is community-driven: https://github.com/catamphetamine/libphonenumber-js/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aclosed+typescript I don't use TypeScript myself. If you happen to find a fix for that then you can post your workaround if it works. Or you can ping other people who previously committed their TypeScript fixes and ask them.

mitas1 commented 3 years ago

The fix for this issue breaks TypeScript json imports:

error TS7016: Could not find a declaration file for module 'libphonenumber-js/examples.mobile.json'. '/node_modules/libphonenumber-js/examples.mobile.json.js' implicitly has an 'any' type. Try npm install @types/libphonenumber-js if it exists or add a new declaration (.d.ts) file containing declare module 'libphonenumber-js/examples.mobile.json';

@mikea did you find a solution for this?

catamphetamine commented 3 years ago

I've added TypeScript "typings" for the JSON files since then. See if those work. Don't npm install @types/libphonenumber-js.

catamphetamine commented 3 years ago

See https://gitlab.com/catamphetamine/libphonenumber-js/

mitas1 commented 3 years ago

Thank you, it turns out I was using older version of this lib.