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

Use sass (dart-sass) instead of node-sass #196

Closed
blurfx opened this issue Jul 19, 2022 · 6 comments · Fixed by #202
Closed

Use sass (dart-sass) instead of node-sass #196

blurfx opened this issue Jul 19, 2022 · 6 comments · Fixed by #202
Assignees
Labels
dependencies Pull requests that update a dependency file good first issue 🐤 Good for newcomers

Comments

@blurfx
Copy link
Contributor

blurfx commented Jul 19, 2022

Description:
Use sass(dart-sass) instead of node-sass.

Why:

  • node-sass is dependent on libsass, which is written in c/c++, so we have to compile using node-gyp, which creates a dependence on the node version.
  • libsass and node-sass is deprecated. See https://sass-lang.com/blog/libsass-is-deprecated
  • The Sass team recommends using the dart-sass, and as far as I know, it is drop-in replacement for node-sass.
@blurfx blurfx added good first issue 🐤 Good for newcomers dependencies Pull requests that update a dependency file labels Jul 19, 2022
@gollumnima
Copy link
Contributor

gollumnima commented Jul 27, 2022

@blurfx Could you assign this issue to me? I'll install sass and update any other libraries like typescript and eslint

@gollumnima
Copy link
Contributor

gollumnima commented Jul 27, 2022

After updating typescript version, I found type errors in src/components/Editor/DrawingBoard/Canvas/dom.ts file.
The problem is Property 'pointerEnabled' and 'msPointerEnabled' don't exist on type 'Navigator'.

/**
 * {@linkcode https://github.com/bevacqua/dragula/blob/e0bcdc72ae8e0b85e17f154957bdd0cc2e2e35db/dragula.js#L498}
 */

export function touchy(el: EventTarget, event: Function, type: MouseType, fn: (evt: TouchyEvent) => void) {
  if (global.navigator.pointerEnabled) { // 🚨🚨🚨🚨🚨🚨🚨
    event(el, pointers[type], fn);
  } else if (global.navigator.msPointerEnabled) { // 🚨🚨🚨🚨🚨🚨🚨
    event(el, microsoft[type], fn);
  } else {
    event(el, touch[type], fn);
    event(el, type, fn);
  }
}

So I've looked into dragula project and found dragula.min.js file.
There was something in the file that seemed to declare a navigator function.

function U(e, t, n, o) {
  r.navigator.pointerEnabled
    ? k[t](
        e,
        {
          mouseup: 'pointerup',
          mousedown: 'pointerdown',
          mousemove: 'pointermove',
        }[n],
        o,
      )
    : r.navigator.msPointerEnabled
    ? k[t](
        e,
        {
          mouseup: 'MSPointerUp',
          mousedown: 'MSPointerDown',
          mousemove: 'MSPointerMove',
        }[n],
        o,
      )
    : (k[t](
        e,
        {
          mouseup: 'touchend',
          mousedown: 'touchstart',
          mousemove: 'touchmove',
        }[n],
        o,
      ),
      k[t](e, n, o));
}

Like this, I think we need to declare our own navigator type in the project.
Or is there any other good choice to fix this issue? 🥲
Or do I roll back typescript to previous version for the backward compatibility?

@ppeeou
Copy link
Contributor

ppeeou commented Aug 1, 2022

@gollumnima
It seems that the dom type has changed a lot since typescript 4.4.
microsoft/TypeScript-DOM-lib-generator#1029
pointerEnabled, msPointerEnabled is used to support older environments (Window)

Initially, we wrote the code to support old browsers
Now, ie no longer needs to be supported, so I think we can delete the code. Also, we have already stated in the package.json browserlist that only the latest browser environment is supported.

@ppeeou
Copy link
Contributor

ppeeou commented Aug 1, 2022

If we delete the code, I think it's better to split the pr. Even if we use 'any' type

@blurfx
Copy link
Contributor Author

blurfx commented Aug 1, 2022

note: please use unknown instead of any if possible :)

@metleeha metleeha mentioned this issue Aug 1, 2022
2 tasks
@metleeha
Copy link
Contributor

metleeha commented Aug 1, 2022

@blurfx Sorry for being late. I was running out of time, so I requested a pr without permission. I simply modified package.json and uploaded it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file good first issue 🐤 Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants