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

Have one distinct build file per language #40

Closed
wants to merge 2 commits into from

Conversation

ludans
Copy link

@ludans ludans commented Nov 21, 2020

Some applications (well, mine actually) only require some languages.
Importing all of them for browser use is too heavy (93kb), while 1 file per language is lighter (30 to 40kb).
This PR builds, along the complete file, 1 file per language in dist/.

@TylerVigario
Copy link
Collaborator

TylerVigario commented Nov 21, 2020

The original idea was if you want a portion of it, you would need to use WebPack (or equivalent) yourself. I did this to keep the NPM package size small and to allow downstream users flexibility.

This PR does not allow the selective inclusion but really just pre-packages each language individually, as you can already selectively include languages.

Do you not run WebPack in your own project?

@ludans
Copy link
Author

ludans commented Nov 21, 2020

Thanks for your feedback.

Yes, this PR does not allow the selective inclusion but really just pre-packages each language individually.
I think it is easier, for a user, to point explicitly to a dedicated language specific language file rather than repackaging - maybe I'm wrong?
And I usually do not care that much about the package size on npm - but I can be wrong too.

(I use rollup)

I'm unsure about how I could remove unuseful stuff from an already precompiled package. Tree shake, probably, won't work.

I'll test tomorrow.

@TylerVigario
Copy link
Collaborator

TylerVigario commented Nov 22, 2020

You can easily include any of the language files directly.

import Num2Word from 'n2words/lib/i18n/EN.mjs';

Edit: Are you perhaps having trouble importing in a CommonJS environment?

@ludans
Copy link
Author

ludans commented Nov 22, 2020

  • It is possible to reference the .mjs file directly, as source code is included in the package, but I would say that it is a questionable practice.

  • .mjs files cannot be included from TypeScript

  • I can create something like that:

n2wordsWrapper.js

const n2words = require('../../../node_modules/n2words/lib/i18n/EN.mjs');

exports.n2wordsEn = function (val) {
  return n2words(val, { lang: 'en' });
};

and include this file in my .ts files

(I don't like it: the path is weird in my mono repo)

  • but then I have a usual SyntaxError: Cannot use import statement outside a module

  • adding "type": "module" to my package.json (even if I don't want to) + run with --experimental-modules (but I don't want to)

  • I now get this error:

n2words/lib/classes/N2WordsAbs.mjs:7
  this.negative_word; //TODO: add support for negative words.
       ^

TypeError: Cannot read property 'negative_word' of undefined

Isn't all that a huge mess? What for? Just including the proper .js in dist should solve it all.

@TylerVigario
Copy link
Collaborator

TylerVigario commented Nov 22, 2020

You can use modules in TypeScript - https://www.typescriptlang.org/docs/handbook/modules.html

As for the invalid reference, it's because of the context this has. It's probably an issue with your RollUp config - rollup/rollup#942

@TylerVigario
Copy link
Collaborator

TylerVigario commented Nov 22, 2020

I see .cjs and .mjs is a problem with TypeScript and there is an open issue to add support - microsoft/TypeScript#27957

Edit: I understand you already found a workaround, however undesirable.

@TylerVigario
Copy link
Collaborator

If you can setup a repo with the minimal amount necessary to reproduce the issue I'd be glad to help further.

@ludans
Copy link
Author

ludans commented Nov 23, 2020

Thanks for your suggestions.

What I do currently:

  • I have added my fork to my mono repo (fork called rosaenlg-n2words)
  • in TypeScript I easily import just the language I need:
import n2words from '../../rosaenlg-n2words/dist/n2words_FR.js';
  • I did not have to change my rollup config

and I end up with smaller packages - which was my sole target.

I will switch to mainstream n2words repo back once I can import easily mjs files in TypeScript. I don't except to do any modifications of n2words except this packaging part.

Feel free to accept or deny the PR. I can live with the fork (even if I will have to sync it manually with the main repo).

@TylerVigario
Copy link
Collaborator

TylerVigario commented Nov 23, 2020

I guess that would work for you, but I don't believe your issue begins at this package.

There should be absolutely no need to consume the bundled versions if you are using a bundler in your own project. The vanilla (source) code runs on latest Node and Browsers with no modification. We run even the tests on source files in Node as-is.

As for providing pre-built individual languages that we bundle and polyfill which is probably only needed for older browsers is up to @forzagreen

IMHO you should build for your specific target environments and steer away from consuming polyfilled scripts.

@ludans
Copy link
Author

ludans commented Nov 23, 2020

I agree with the way of packaging.
I still think I'm stuck with TypeScript, probably during a few months.
I'll try to simplify my packaging process (and remove my forked version of n2words) in a few months.

@TylerVigario
Copy link
Collaborator

I believe you are on to something.

We should give concern to CommonJS users wanting only specific languages. Currently, our build process only creates a concatenated script that includes every language.

Interoperability between CJS and ESM is still murky water, and I will need to refine it. I will look at improving the dual environment package within the next couple months as well.

@TylerVigario
Copy link
Collaborator

TylerVigario commented Nov 25, 2020

I believe this to be the source of truth - https://nodejs.org/api/esm.html#esm_interoperability_with_commonjs

Correct me if Node doesn't set the standards.

I found this in the current documentation:
The CommonJS module require always treats the files it references as CommonJS.

as well as
Using require to load an ES module is not supported because ES modules have asynchronous execution. Instead, use use import() to load an ES module from a CommonJS module.

Yes, they doubly used "use". LOL

Futher more, I will create a vanilla package in CJS and try to include the ESM languages directly. If I encounter any issues, I will figure out a solution and make it known.

@ludans
Copy link
Author

ludans commented Nov 25, 2020

thanks, I'm eager to see the solution (and integrate it)

@TylerVigario
Copy link
Collaborator

TylerVigario commented Nov 29, 2020

I successfully imported the ESM module via CommonJS with the import() function.

// Dynamic Import
import('n2words/lib/n2words.mjs').then(n2words => {
    // Available via "default" function
    console.log(n2words.default(100));
}).catch(err => {
    // Catch dynamic import error
    console.error(err);
});

@ludans
Copy link
Author

ludans commented Nov 29, 2020

I have a very, very small package written in TypeScript and using n2words - actually only the Italian locale.

npm package is https://www.npmjs.com/package/italian-ordinals-cardinals
source is here https://github.com/RosaeNLG/rosaenlg/tree/master/packages/italian-ordinals-cardinals (in RosaeNLG mono repo)

I just can't make it work using standard version of n2words.

@TylerVigario
Copy link
Collaborator

TylerVigario commented Nov 29, 2020

Unfortunately, I had to use a forked version of TypeScript to resolve the issues it has with .mjs files - https://github.com/stealify/typescript

@forzagreen forzagreen mentioned this pull request Jul 23, 2021
@TylerVigario
Copy link
Collaborator

This should be closed IMHO

@ludans ludans closed this Mar 25, 2022
@forzagreen
Copy link
Owner

@ludans, There has been recently some work related to this topic by @TylerVigario
Please check https://github.com/forzagreen/n2words/wiki/Importing-only-specific-languages

@ludans
Copy link
Author

ludans commented Jun 3, 2023

thanks for the notification
I made a test and have a typescript issue, see #109

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