search

material-foundation / material-color-utilities

Color libraries for Material You
Apache License 2.0
1.57k stars 134 forks source link

fix: work on node + browser #61

Closed WillsterJohnson closed 1 year ago

WillsterJohnson commented 1 year ago

Currently the only working version of this lib is @importantimport/material-color-utilities

Fixes #35 Fixes #68

This PR allows the NPM package to work on all modern browsers and on Node, where currently it does not work anywhere.

To maintain the correct behavior of the JS code being able to run in JS environments, ensure that all import statements point to a *.js file.

Recommended version bump: Minor Reason: adds new functionality;

google-cla[bot] commented 1 year ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

WillsterJohnson commented 1 year ago

Currently the typescript library doesn't work on standard platforms (node, browser) due to the lack of .js extensions.

This has been an issue for at least 7 months.

Currently, the best way to use material color utilities is to completely ignore this official source and instead rely on 3rd party forks which have done the simple task of appending .js to file imports which enables this library's code to run.

See:

35 , #68

@material-admin without this PR, the TypeScript library does not work @kluever copying you in as you're the only person who's committed to this repo in the last 6 months

WillsterJohnson commented 1 year ago

The issue fixed by this PR was first created in this commit, meaning today marks 440 (four hundred forty) days of the TypeScript library being defunct on all JavaScript runtime environments.

See: #35, #68

@material-admin, without this PR, the TypeScript library does not work @kluever @guidezpl

kwaa commented 1 year ago

It seems that Google apparently doesn't care much about the availability of this library... or they ship TypeScript directly internally.

WillsterJohnson commented 1 year ago

@kwaa It's quite common of Google to do this. Create something good (the Material You design system), then take a big ol' dump on it (this 467-day old issue rendering the library useless). Such a shame, if they fixed these problems with their softwares they would probably push MS and Apple into obscurity.

rodydavis commented 1 year ago

LGTM

WillsterJohnson commented 1 year ago

pulled updates to the fork & fixed newly introduced fatal syntax errors. @rodydavis , please review these changes if you have any concerns.

As this repo is a mirror, here is a bash command which will automatically apply the changes this PR makes;

echo "const f=require('fs');const p=require('path');const x=(d)=>{for(const t of f.readdirSync(d)){if(t.endsWith('.ts'))f.writeFileSync(p.join(d,t),f.readFileSync(p.join(d,t),'utf8').replace(/(im|ex)port(.+from.+?'.+)(?<!(\.js|jasmine))';/g,'$1port$2.js\';'),'utf8');else if(f.statSync(p.join(d,t)).isDirectory())x(p.join(d,t))}};x(process.cwd());" | node

If it turns out that the Material Foundation dev team for whatever reason won't include the .js extension that is required for ESM code to execute in a JavaScript runtime environment, then it would be a good idea to put in place some kind of enforcement.

Be it a github workflow

name: Fix fatal coding practice

on:
  push:
    branches:
      - "main"

jobs:
  fixfatalerrors:
    runs-on: ubuntu-latest

    steps:
      - name: Checkout
        uses: actions/checkout@v3

      - name: Setup Node
        uses: actions/setup-node@v3
        with:
          node-version: "18.x"
          registry-url: "https://registry.npmjs.org"

      - name: Fix Fatal Issues
        run: echo "const f=require('fs');const p=require('path');const x=(d)=>{for(const t of f.readdirSync(d)){if(t.endsWith('.ts'))f.writeFileSync(p.join(d,t),f.readFileSync(p.join(d,t),'utf8').replace(/(im|ex)port(.+from.+?'.+)(?<!(\.js|jasmine))';/g,'$1port$2.js\';'),'utf8');else if(f.statSync(p.join(d,t)).isDirectory())x(p.join(d,t))}};x(process.cwd());" | node

Or a pre-commit hook

#!/usr/bin/env sh

git stash

echo "const f=require('fs');const p=require('path');const x=(d)=>{for(const t of f.readdirSync(d)){if(t.endsWith('.ts'))f.writeFileSync(p.join(d,t),f.readFileSync(p.join(d,t),'utf8').replace(/(im|ex)port(.+from.+?'.+)(?<!(\.js|jasmine))';/g,'$1port$2.js\';'),'utf8');else if(f.statSync(p.join(d,t)).isDirectory())x(p.join(d,t))}};x(process.cwd());" | node

git add .

git stash pop

echo "\x1b[35mFIXED FATAL ISSUES IN THE CODEBASE: SEE STAGED CHANGES"

Or anything else. The key thing here is for the last 476 days (or longer) the typescript shipped from this repo (potentially others) has been categorically unusable and defunct, and that desperately needs to change.

Material design is, in my opinion, the best design system by a long shot. I, and many others, would very much like it if the tools made available to us did not cause our applications to crash immediately upon startup, or worse, at any point during runtime.

I hope I've made it clear how frustrating it has been to not be able to use something I believe to be such high quality due to such a basic syntactic error.

It would surprise me very much if this repo is the only one effected by this issue, particularly as the same developers working on the TypeScript here are almost certainly working on TypeScript elsewhere. It may be a good idea to check basically every TypeScript file belonging to google that the contributors to this project have been involved with as the same fatal error may be present. As I said recently, if Google & it's child organisations fixed all these little errors (for context, the JS line, workflow, and pre-commit hook took me a combined total of 30 minutes to create, writing a .js extension in the first place is automated by every text editor unless you go out of your way to disable that) I'm certain that Google would utterly dominate every digital space it entered.

mateoroldos commented 1 year ago

+1 🙏 @kluever

pennzht commented 1 year ago

Thank you for your PR. I'm taking a look.

pennzht commented 1 year ago

Thank you for your PR!

While we currently don't accept external contributions, we appreciate that you have brought this to our attention.

We have made this change as https://github.com/material-foundation/material-color-utilities/commit/41cf95d23acb9c1a9248257dc4571e27c502f795 (also extending the change to newly added files).

We are closing this PR and related issues. Thank you for your support.

rodydavis commented 1 year ago

Will update the internal script. Looks like it skipped this file

WillsterJohnson commented 1 year ago

Awesome, I guess #82 will be short-lived. I'll still go ahead and make a list of misses, maybe this was the edge case, maybe the update to the internal script will have new ones. Better safe than sorry. That appears to be the only local import/export from which was missed!