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

fix: work on node + browser #61

Closed
wants to merge 4 commits into from
Closed

fix: work on node + browser #61

wants to merge 4 commits into from

Conversation

WillsterJohnson
Copy link

@WillsterJohnson WillsterJohnson commented Oct 11, 2022

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


  • passes all test cases
  • TypeScript builds successfully
  • does not break existing behavior

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;

  • compatibility with Node.js (LTS, 16+)
  • compatibility with Gecko browser (FireFox 60+)
  • compatibility with Chromium browser (Chrome 61+)
  • compatibility with WebKit browser (Safari 11+)

@google-cla
Copy link

google-cla bot commented Oct 11, 2022

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
Copy link
Author

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
Copy link
Author

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
Copy link

kwaa commented Jan 24, 2023

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

@WillsterJohnson
Copy link
Author

@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
Copy link
Member

LGTM

@WillsterJohnson
Copy link
Author

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
Copy link

+1 🙏 @kluever

@pennzht
Copy link
Collaborator

pennzht commented Feb 8, 2023

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

@pennzht
Copy link
Collaborator

pennzht commented Feb 16, 2023

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 41cf95d (also extending the change to newly added files).

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

@@ -15,9 +15,9 @@
* limitations under the License.
*/

import './point_provider';
import './point_provider.js';
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typescript/quantize/lab_point_provider.ts:18 - .js extension added in this PR but not added in internal change.

@rodydavis
Copy link
Member

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

@WillsterJohnson
Copy link
Author

WillsterJohnson commented Mar 2, 2023

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants