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

chore: Convert to Deno #17

Open
wants to merge 42 commits into
base: master
Choose a base branch
from
Open

Conversation

ebebbington
Copy link

@ebebbington ebebbington commented Jun 3, 2021

Many changes:

  • Removes eslint, and uses deno lint instead
  • Option to use deno fmt is available
  • Removed all npm dependencies, and replaces scripts with scripts for deno
  • Creates a new compile.ts file, in place of tsc, which uses 2 unstable api's
  • Adds .ts extensions to imports for src instead of .js (to pass emit checks)
  • Remove tsconfig.json and package-lock.json as they are no longer needed
  • A new examples_mod.ts inside src so we can properly emit the examples. Reason for this is, they aren't part of the dependency free for mod.ts so aren't included in the emit result. Due to this, we compile examples_mod.ts too. Not worth compiling each individual file instead because that takes a long time. As it stands, running compile.ts is faster than tsc/webpack

Of course it's completely down to you @0kku, as to whether you want to request changes, let along approve this PR, it's all welcomed and i completely understand

So how do you, Okku, and an end user use destiny now?

Good question edward

As the maintainer of Destiny, you would:

  • Setup a webhook to deno.land, now anyone using Deno can import the src/*.ts files fine
  • npm run compile and deploy as you usually would this generates maps, declaration files and js files, so you can import it in the browser for js files

As a user of Destiny:

  • Import destiny from deno.land
  • or import destiny from the destiny cdn

TODO

compile.ts Outdated Show resolved Hide resolved
@@ -1,3 +1,5 @@
import "../globalTypes/CSSStyleSheet.ts"
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be import type?

Copy link
Owner

Choose a reason for hiding this comment

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

Apparently import type is not supported without importing something specific. However, the way it is in its current form is not ideal, because it'll be importing this file even at runtime if it's not a type import.

Copy link
Author

Choose a reason for hiding this comment

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

This is more a way around using a globalThis.d.ts - it's just to include that type in the global scope

Saying this, where does it mention replace exists on CSSStyleSheet? Not the one ou define, but I remember you saying that the lib doesn't have it defined (yet) but it does exist, or something along those lines?

Copy link
Owner

Choose a reason for hiding this comment

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

Saying this, where does it mention replace exists on CSSStyleSheet? Not the one ou define, but I remember you saying that the lib doesn't have it defined (yet) but it does exist, or something along those lines?

Here's the MDN page and here's some additional documentation.

Copy link
Owner

Choose a reason for hiding this comment

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

The problem with import "./path" is that it shows up in the final compiled output, which is why type-only imports need to be imported with import type to avoid there being extraneous junk in the output.

Copy link
Author

@ebebbington ebebbington Jun 6, 2021

Choose a reason for hiding this comment

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

Ok gotcha, looked through the code and can understand this better

I don't think importing it as a type is a possibility, and if you wanna avoid junk then i think the next best thing would be slapping a ts ignore on it :/ But ill keep thinking

Copy link
Author

Choose a reason for hiding this comment

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

Or another 'hack' would be modifying that global type file to just export an interface, and do something like:

// globaltypes/cssstylesheet.ts
export intrface ICSSStyleSheet extends CSSStyleSheet {
  replace: (css: string) => void
}

// styling/CSSTemplate.ts
import type { ICSSStyleSheet } from ...

...

  (this.#stylehsheet as ICSSStyleSheet).replace(this.cssText)

Copy link
Author

Choose a reason for hiding this comment

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

or do #stylesheet: ICSSStyleSheet | undefined instead (keeping the interface we import too)

Copy link
Owner

Choose a reason for hiding this comment

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

Seems like doing the cast might be the least bad option.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

@ebebbington ebebbington left a comment

Choose a reason for hiding this comment

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

formatting wasn't part of the project before, so remove this

package.json Outdated
"lint:code": "eslint src",
"lint:types": "tsc --noEmit",
"lint": "deno lint --ignore=dist",
"fmt": "deno fmt --ignore=dist",
Copy link
Author

Choose a reason for hiding this comment

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

remove this

Copy link
Owner

Choose a reason for hiding this comment

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

Don't remove it yet, I'll have a look at what it looks like and see if it's useful to keep once I get some time.

@0kku 0kku marked this pull request as draft June 6, 2021 20:50
@ebebbington ebebbington changed the title chore: Convert to Deno [DRAFT] chore: Convert to Deno Jun 7, 2021
@0kku
Copy link
Owner

0kku commented Jul 1, 2021

Love the latest commit message lol.

compile.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
node/package-lock.json
Copy link
Author

Choose a reason for hiding this comment

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

Purely making a note here so I remember to remove the examples directory - would be great if we could turn these into tutorials :)

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah honestly the examples don't make a ton of sense as part of the lib core, but that's where I run the tests, so I don't know if it's feasible to remove the examples completely 😬

Copy link
Author

Choose a reason for hiding this comment

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

It was more to help both us understand if it works for each environment :) After all we could turn the 'setups' into written tutorials

though i see what you mean...might be worth something to you, just maybe there's a better place to put these directories?

Copy link
Owner

Choose a reason for hiding this comment

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

I might've actually misunderstood what you meant, my bad. Ignore my previous comment.

Copy link
Author

Choose a reason for hiding this comment

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

resolving, pr desc now includes a note about this

@ebebbington ebebbington changed the title [DRAFT] chore: Convert to Deno chore: Convert to Deno Jul 1, 2021
@ebebbington ebebbington marked this pull request as ready for review July 1, 2021 20:58
@ebebbington
Copy link
Author

as of now, using canary (> 1.11.5) the infinite loop has gone, but revealed some more issues:

denoland/deno#11286
denoland/deno#11287

@ebebbington
Copy link
Author

3647b7f does address both issues mentioned above, though of course it is a tiny workaround

@0kku
Copy link
Owner

0kku commented Jul 6, 2021

3647b7f does address both issues mentioned above, though of course it is a tiny workaround

Could've just moved the cleanup method outside the class, since it's doing nothing with the instance anyway.

@ebebbington
Copy link
Author

3647b7f does address both issues mentioned above, though of course it is a tiny workaround

Could've just moved the cleanup method outside the class, since it's doing nothing with the instance anyway.

Guess that's your preferred way then :P Is this approach something you're ok with?

Comment on lines 1 to 8
// Import some example files that aren't included in the dependency free, so we can compile them
import "./_examples/main.ts"
import "./_examples/components/visitor-demo.ts"
import "./_examples/components/to-do/_to-do.ts"
import "./_examples/components/array-demo.ts"
import "./_examples/components/time-diff.ts"
import "./_examples/components/async-demo.ts"
import "./_examples/components/window-manager/_window-manager.ts"
Copy link
Author

Choose a reason for hiding this comment

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

Note about this file that i want to mention:

If instead of setting the path to the js file inside the tab-view.ts, we imported the class and passed it in, we wouldn't need the examples_mod.ts file, and would only need to emit _examples/main.ts, so for example:

// src/_examples/components/tab-view.ts
import { AllComponents } from "all the files we need"

class TabView {
  #tabs = [
    {
      url: "/array-demo",
      component: ArrayDemo // before, it would be "./components/array-demo.js"
    }
  ]
}

Of course the hash router would need to account for this.

Curious on your thoughts @0kku?

Copy link
Owner

Choose a reason for hiding this comment

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

That would make it into a static import. It's intentionally dynamic: the component is not loaded until the user clicks on the tab. This makes initial load faster and avoids loading a potentially substantial number of pages that the user wasn't interested in visiting anyway. It's not a big deal on the example page because it's very small, but it's an example on how to do code splitting for a large website that could potentially have hundreds of large pages.

Copy link
Author

Choose a reason for hiding this comment

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

very very good point 👍

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.

2 participants