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

Move to TypeScript #1750

Merged
merged 117 commits into from Feb 8, 2019

Conversation

Projects
None yet
6 participants
@nickytonline
Copy link
Contributor

nickytonline commented Feb 1, 2019

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

Show resolved Hide resolved source/globals.d.ts Outdated
Show resolved Hide resolved source/globals.d.ts Outdated
Show resolved Hide resolved source/libs/api.ts Outdated
Show resolved Hide resolved source/libs/api.ts Outdated
Show resolved Hide resolved source/globals.d.ts Outdated

@nickytonline nickytonline force-pushed the nickytonline:typescript-migration branch from 3589fe1 to fa0610e Feb 1, 2019

@nickytonline

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Feb 1, 2019

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

@nickytonline

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Feb 1, 2019

This WIP PR implements #1735.

Show resolved Hide resolved package.json Outdated
Show resolved Hide resolved source/globals.d.ts Outdated
Show resolved Hide resolved source/libs/api.ts Outdated
Show resolved Hide resolved source/libs/api.ts
Show resolved Hide resolved source/types/browser.d.ts Outdated
Show resolved Hide resolved source/types/globals.d.ts Outdated
Show resolved Hide resolved source/types/globals.d.ts Outdated

@nickytonline nickytonline force-pushed the nickytonline:typescript-migration branch from 3928990 to 5aafdc6 Feb 2, 2019

@@ -9,10 +9,10 @@ function init() {

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

This comment has been minimized.

@nickytonline

nickytonline Feb 2, 2019

Author Contributor

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.

This comment has been minimized.

@bfred-it

bfred-it Feb 2, 2019

Collaborator

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

This comment has been minimized.

@sindresorhus

sindresorhus Feb 2, 2019

Owner

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

This comment has been minimized.

@nickytonline

nickytonline Feb 2, 2019

Author Contributor

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.

This comment has been minimized.

@nickytonline

nickytonline Feb 2, 2019

Author Contributor

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

image

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.

image

This comment has been minimized.

@bfred-it

bfred-it Feb 2, 2019

Collaborator

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?

This comment has been minimized.

@nickytonline

nickytonline Feb 2, 2019

Author Contributor

It works in the browser but not in TS land.

image

image

This comment has been minimized.

Show resolved Hide resolved source/libs/utils.js Outdated
@nickytonline

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Feb 2, 2019

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.

Show resolved Hide resolved source/libs/page-detect.ts Outdated
@bfred-it

This comment has been minimized.

Copy link
Collaborator

bfred-it commented Feb 2, 2019

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 😬

@bfred-it

This comment has been minimized.

Copy link
Collaborator

bfred-it commented Feb 2, 2019

I think webpack’s config can stay JavaScript

@nickytonline

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Feb 2, 2019

Sounds good for strict coming in another round.

Show resolved Hide resolved source/libs/utils.ts Outdated
@@ -9,10 +9,10 @@ function init() {

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

This comment has been minimized.

@nickytonline

nickytonline Feb 2, 2019

Author Contributor

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.

@@ -9,10 +9,10 @@ function init() {

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

This comment has been minimized.

@nickytonline

nickytonline Feb 2, 2019

Author Contributor

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

image

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.

image

@bfred-it

This comment has been minimized.

Copy link
Collaborator

bfred-it commented Feb 6, 2019

I'm trying something. Hold on the commits

@bfred-it bfred-it force-pushed the nickytonline:typescript-migration branch 2 times, most recently from 02469bb to e4262e6 Feb 6, 2019

@bfred-it

This comment has been minimized.

Copy link
Collaborator

bfred-it commented Feb 6, 2019

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

@nickytonline

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Feb 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Feb 7, 2019

@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.

@bfred-it

This comment has been minimized.

Copy link
Collaborator

bfred-it commented Feb 7, 2019

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

  1. Make this PR include all the changes, except the renames of the three files
  2. 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:

  1. I'll undo the renames
  2. Merge the PR
  3. Commit the renames
@nickytonline

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Feb 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Feb 7, 2019

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

@JesseKPhillips

This comment has been minimized.

Copy link

JesseKPhillips commented Feb 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

nickytonline commented Feb 7, 2019

@bfred-it

This comment has been minimized.

Copy link
Collaborator

bfred-it commented Feb 7, 2019

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

This comment has been minimized.

Copy link

JesseKPhillips commented Feb 7, 2019

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.

bfred-it added some commits Feb 8, 2019

@bfred-it bfred-it merged commit 9f9d178 into sindresorhus:master Feb 8, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@bfred-it

This comment has been minimized.

Copy link
Collaborator

bfred-it commented Feb 8, 2019

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:

screenshot 2019-02-08 at 22 57 14

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

@bfred-it

This comment has been minimized.

Copy link
Collaborator

bfred-it commented Feb 8, 2019

Thanks @nickytonline for the hard work! 🎈

all typescript

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! 🎂

1000 commits

@JesseKPhillips

This comment has been minimized.

Copy link

JesseKPhillips commented Feb 8, 2019

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

@nickytonline nickytonline deleted the nickytonline:typescript-migration branch Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.