Move to TypeScript by nickytonline · Pull Request #1750 · refined-github/refined-github

nickytonline · 2019-02-01T05:29:00Z

I'm just opening the PR right away @sindresorhus, so that I can get back any feedback from you as I move along.

nickytonline · 2019-02-01T06:20:32Z

I'll work on this some more tomorrow evening and probably the weekend. 👋 for now.

nickytonline · 2019-02-01T06:39:29Z

This WIP PR implements #1735.

nickytonline · 2019-02-02T04:57:04Z

 	for (const comment of comments) {
 		const timestampEl = select('relative-time', comment);
-		const timestamp = timestampEl.attributes.datetime.value;
+		const timestamp = timestampEl.attributes['datetime'].value; // eslint-disable-line dot-notation


I disabled the dot-notation rule here because the type checking fails otherwise. I know it works in JS land, but the types are strict, so it's required to use [] notation because in the generic sense, not every element has an attributes.datetime property.

I think the correct way would be to cast select<HTMLTimeElement> or whatever element GitHub’s relative-time extends

You should cast it to the correct element instead, after the “select” call.

Sounds good. I feel silly for not knowing there was a time element, https://developer.mozilla.org/en-US/docs/Web/HTML/Element/tim. Will cast accordingly.

Looks like even when it is cast to HTMLTimeElement, it still complains.

It's actually the <relative-time /> web component from github.com/github/time-elements that they're using, so you have no choice but to access it via attributes with the [] notation.

Maybe the issue is that time actually has a dateTime attribute: https://developer.mozilla.org/en-US/docs/Web/API/HTMLTimeElement (camelcase)

const timestampEl = select<HTMLTimeElement>('relative-time', comment); const timestamp = timestampEl.attributes.dateTime.value;

Does that work?

It works in the browser but not in TS land.

Context: https://github.com/Microsoft/TypeScript/blob/9d3707d671592d030386956c9ce39e539b8d0972/src/lib/dom.generated.d.ts#L4757

I give up 🤷‍♂️

nickytonline · 2019-02-02T06:43:09Z

Made some good progress today. Only one file left to convert in source/libs and then four more in the root of the source folder. There's two tests that I broke that I'll fix as well.

After that I'll enable strict mode and see what still breaks. Hopefully not too much at this point ;)

Do you want your webpack config converted to TypeScript as well? All it requires is the ts-node package and webpack will be able to transpile the config before building.

fregante · 2019-02-02T11:42:13Z

As for the strict mode, if it requires a lot more changes I’d probably suggest to settle this PR first. Making the repo strict could come in a separate round. What do you think?

Also I found some solutions in the other PR to be slightly more simple, especially where you use the TailArgs here 😬

fregante · 2019-02-02T13:19:33Z

I think webpack’s config can stay JavaScript

nickytonline · 2019-02-02T13:54:43Z

Sounds good for strict coming in another round.

nickytonline · 2019-02-02T14:47:27Z

 	for (const comment of comments) {
 		const timestampEl = select('relative-time', comment);
-		const timestamp = timestampEl.attributes.datetime.value;
+		const timestamp = timestampEl.attributes['datetime'].value; // eslint-disable-line dot-notation


Sounds good. I feel silly for not knowing there was a time element, https://developer.mozilla.org/en-US/docs/Web/HTML/Element/tim. Will cast accordingly.

nickytonline · 2019-02-02T16:10:31Z

 	for (const comment of comments) {
 		const timestampEl = select('relative-time', comment);
-		const timestamp = timestampEl.attributes.datetime.value;
+		const timestamp = timestampEl.attributes['datetime'].value; // eslint-disable-line dot-notation


Looks like even when it is cast to HTMLTimeElement, it still complains.

It's actually the <relative-time /> web component from github.com/github/time-elements that they're using, so you have no choice but to access it via attributes with the [] notation.

fregante · 2019-02-06T06:32:59Z

I don't know either. I tried something and it failed, even if I reset the changes and then rename the files: 5c41a72

nickytonline · 2019-02-06T06:40:30Z

Hmm. I have to get some ZZZs, but I'll pick the brains of a few colleagues tomorrow to see if they might know what's up.

nickytonline · 2019-02-07T02:55:18Z

Tried again a little differently, but same result. It renames the file properly, I commit it and push and I see it's renamed in the PR, but as soon as the next commit comes in that boom, same thing.

This time I checked out the file at the exact place where rename fiasco happened

git rev-list -n 1 HEAD -- source/libs/icons.js # gives 5ea00a3923adaee0bbd1114a74be21c8ab5c1726
git checkout 5ea00a3923adaee0bbd1114a74be21c8ab5c1726^ -- source/libs/icons.js
# delete source/libs/icons.tsx and commit.
git mv source/libs/icons.js source/libs/icons.tsx
# file is committed and is shown as a rename.

# modify contents of icons.tsx in a new commit and we're back to square one.

ahhh! 😅

nickytonline · 2019-02-07T03:01:00Z

@sindresorhus just curious if you have any ideas as to why we're unable to rename the files properly. From all accounts, I'm doing things correctly which is why I'm a little perplexed.

fregante · 2019-02-07T03:15:46Z

I think the solution is to just merge as two commits.

Make this PR include all the changes, except the renames of the three files
After merging the PR, commit the file rename directly on master.

This way GitHub won't try to squash/merge the two commits incorrectly.

So let's review this the final time and then:

I'll undo the renames
Merge the PR
Commit the renames

nickytonline · 2019-02-07T03:20:00Z

OK, sounds good. Thanks for all the help.

The only thing that is missing is we need the latest webext-options-sync as well in here. It works right now, but it'd be good to get that in as well. What do you think?

nickytonline · 2019-02-07T03:24:00Z

It needs the updated package-lock.json. I can push that if you want.

JesseKPhillips · 2019-02-07T05:20:41Z

Git does not have a rename, make a commit without file modification but includes both the add and delete on the file. Git will use horistics to determine the files are the same.

nickytonline · 2019-02-07T05:39:02Z

Here's some insight into the issue @bfred-it,
https://dev.to/nickytonline/renaming-a-file-via-git-after-it-has-been-deleted-in-a-pr-41dn

fregante · 2019-02-07T07:27:36Z

I read that before that “git does not have renames” but it’s kinda wrong considering that a rename can be --followed by git log and rm+add can’t. Right? There’s a difference between the two operations, even if ”technically” git just uses rm+add

JesseKPhillips · 2019-02-07T15:13:36Z

Nope no difference, their is no meta data to indicate a rename. As you said, git mv just uses add+remove. I recently had a collegue hit the opposite problem, he had added a new file, copied content from another and deleted the old. Git saw it as a rename but he didn't expect that because that wasn't the operation done.

fregante · 2019-02-08T14:59:10Z

9f9d178 merged with file changes
c11d1f7 committed with the renames

Now use something like Follow for Github or git log --follow and you'll see this:

It turns out you git does have the concept of renames, you just have to keep the changes and the renames in separate commits.

fregante · 2019-02-08T15:02:31Z

Thanks @nickytonline for the hard work! 🎈

I merged this to allow other PRs to start following the new rules. Further adjustments can be made in a separate PR.

And just like that... 1000 commits! 🎂

JesseKPhillips · 2019-02-08T15:16:14Z

Git does have a concept of rename with file modifications, it is just harder which is why I specified to have them separate, and when I looked at the history they were.

https://www.quora.com/What-is-the-heuristic-of-Git-for-managing-renamed-files

trddddd · 2019-10-02T19:32:06Z

you can rewrite history where this file was mentioned

git filter-branch -f --tree-filter 'if [ -f old_file ]; then mkdir -p new_dir && mv old_file new_file; fi'
mv old_file new_file
gaa && gcmsg 'perfect'

https://gist.github.com/trddddd/69f1f134499b4e1aaf84166911a2830c

raineorshine · 2020-08-06T21:46:10Z

For those wanting to understand better why git intentionally does not track renames:

https://git.wiki.kernel.org/index.php/GitFaq#Why_does_Git_not_.22track.22_renames.3F

nickytonline commented Feb 1, 2019

View reviewed changes

Comment thread source/globals.d.ts Resolved

nickytonline commented Feb 1, 2019

View reviewed changes

Comment thread source/globals.d.ts Resolved

nickytonline commented Feb 1, 2019

View reviewed changes

Comment thread source/globals.d.ts Resolved

nickytonline commented Feb 1, 2019

View reviewed changes

Comment thread source/libs/api.ts Resolved

nickytonline commented Feb 1, 2019

View reviewed changes

Comment thread source/libs/api.ts Resolved

nickytonline commented Feb 1, 2019

View reviewed changes

Comment thread source/globals.d.ts Resolved

nickytonline force-pushed the typescript-migration branch from 3589fe1 to fa0610e Compare February 1, 2019 06:18

nickytonline commented Feb 1, 2019

View reviewed changes

Comment thread source/libs/api.ts Resolved

fregante requested changes Feb 1, 2019

View reviewed changes

Comment thread package.json Resolved

Comment thread source/globals.d.ts Resolved

Comment thread source/libs/api.ts Resolved

Comment thread source/libs/api.ts Resolved