Remove hasteResolver in favor of standard esm relative imports [Fix #513] #519

boxfoot · 2020-03-05T16:06:53Z

Description

Fixes #513 by removing the old "hasteResolvers" and using standard relative imports instead.

Motivation and Context

The hasteResolvers is a legacy from when FB managed this package. While it makes imports a little bit more concise, it obscures the architecture of the overall package and makes it harder for new contributors to understand how things fit together. It's also a bit of magic that is only understand by webpack, making the code harder to maintain and reuse. In contrast, relative imports are an old standard way of locating files on a file system and is totally supported by node.

Some notes/questions:

Making this change emphases that there are a bunch of utilities and polyfills included under vendor_upstream, some of which are probably not needed anymore (like requestAnimationFrame, others that are duplicative (we sometimes use vendor_upstream/core/clamp and sometimes lodash/clamp and they both do the same thing) and others that could be easily provided by small npm packages. I'm leaving making any of those changes out of scope for now, but we may want to address later. For now, I've grouped imports into three groups in each file: (1) raw package imports, (2) vendor_upstream imports, (3) local relative imports.
Along the way I made some small changes to jsdoc/ts typings to silence errors I was getting in my IDE, and also added some @types/ to the devDependencies for this purpose
My code formatter also made some changes to whitespace, mostly making things more consistent within the repo (like removing invisible spaces) but if you don't want these, LMK and I can revert

How Has This Been Tested?

Ran all tests, tried out examples.

Screenshots (if appropriate):

Types of changes

Non-feature refactor to simplify code

Checklist:

My code follows the code style of this project.
~~My change requires a change to the documentation.~~
~~I have updated the documentation accordingly.~~
I have read the CONTRIBUTING document.

wcjordan · 2020-03-09T14:48:10Z

Thanks for putting this PR up! It looks like the tests are failing on travis-ci, have you had a chance to look into the cause of that yet?

wcjordan

Looks good to me once travis is fixed and the formatting nits are addressed.

Thank you for the formatting fixes, they're much appreciated and needed. In future PRs could you separate the formatting changes to a different PR from the content so it's easier to review?

wcjordan · 2020-03-09T14:52:30Z

src/vendor_upstream/dom/DOMMouseMoveTracker.js

@@ -73,7 +73,7 @@ class DOMMouseMoveTracker {
    }

    if (this._isTouchEnabled && !this._eventTouchStartToken &&
-        !this._eventTouchMoveToken && !this._eventTouchEndToken) {


nit: add this spacing back. We don't want the if conditional and the block statement content to be at the same indent

wcjordan · 2020-03-09T14:52:57Z

src/vendor_upstream/dom/DOMMouseMoveTracker.js

@@ -118,7 +118,7 @@ class DOMMouseMoveTracker {
    }

    if (this._isTouchEnabled && this._eventTouchStartToken &&
-        this._eventTouchMoveToken && this._eventTouchEndToken) {


nit: same spacing comment as above

wcjordan · 2020-03-09T15:00:06Z

src/vendor_upstream/stubs/EventListener.js

@@ -25,18 +25,18 @@ var EventListener = {
   * @param {function} callback Callback function.
   * @return {object} Object with a `remove` method.
   */
-  listen: function(target, eventType, callback) {
+  listen: function (target, eventType, callback) {


nit: don't add this space between the function keyword and the parameters. Here and throughout the files.

boxfoot · 2020-03-09T16:03:29Z

@wcjordan thanks for the review! I believe all addressed

It looks like the tests are failing on travis-ci

This was caused by unrelated changes pulled in from #507 that I hadn't noticed. Now fixed.

Thank you for the formatting fixes, they're much appreciated and needed. In future PRs could you separate the formatting changes to a different PR from the content so it's easier to review?

I can totally do in the future. In this case, they weren't planned, just a side-effect of my IDE auto-formatting files as I worked on them. (The various nits were also disagreements between my IDE's defaults and the standards in this repo). Makes me wonder if it would be worth defining prettierrc and/or eslintrc for this repo and perhaps even hooking into commits with e.g. husky so that formatting is frictionless and automatic.

wcjordan · 2020-03-09T19:52:34Z

We definitely would like to have prettier run over the repo and a sane eslint would also be valuable as well. There's some discussion on #218 & #455

wcjordan · 2020-03-17T22:50:23Z

@pradeepnschrodinger I'm ready to merge this given the formatting concerns are addressed by #520. Do you have any concerns? Should we wait until after v1.1 is released?

pradeepnschrodinger

This looks very good, thanks.
I see some troubles with the build process, but don't feel like we should block the merge.

But unless the build is fixed, we can't release right? So either we can merge right away, and hold of v1.1 release for some time until we figure out the build changes, or we just merge this after the release.

wcjordan · 2020-03-23T20:53:12Z

Let's hold off merging and get 1.1 released while we work the issues out w/ our NPM package. Niranjan & I are discussing the NPM issues and will follow up here soon.

boxfoot · 2020-03-23T20:56:54Z

Sounds good. LMK if there's something I missed or can fix to get the build back online.

wcjordan · 2020-03-23T20:59:20Z

Will do. A lot of the issue has to do with how this buildNPMInternals.sh script flattens the directory structure when preparing to upload to NPM. We're discussing what we want the outcome to be, and will reach out once we know that (or in general details of why this old build system is how it is).

wcjordan · 2020-05-11T12:12:38Z

Hey @boxfoot sorry for our delays here. We had some other priorities come up and haven't been able to investigate fixing buildNPMInternals.sh yet. I just wanted to know we haven't forgotten about this, but it may be another month before we have a good answer.

wcjordan · 2020-07-21T14:07:20Z

Hey @boxfoot sorry again for all our delays here. We've been heads down on some other projects, and are just now getting back to this. We should be making progress over the coming few weeks.

pradeepnschrodinger · 2020-08-11T07:15:03Z

@boxfoot , hey really sorry for the delay.
Our current build system uses a flat directory hierarchy, so it won't work with relative imports. Good thing is that if we just change how the build script arranges the files, the build works fine.

So could you merge squash the commit 8cff569 to this branch? The commit will update the build script to be compatible with relative imports, and then this PR would be good to go.

Also, there seems there's a few merge conflicts with latest master. Could you also take care of that?

pradeepnschrodinger · 2020-09-22T12:54:18Z

@boxfoot , merged this PR to master through 8677056.
The follow up commit (1bcf8a2) fixes the build issues.

boxfoot · 2020-09-22T18:44:26Z

@pradeepnschrodinger Thanks for the update!

pradeepnschrodinger requested review from pradeepnschrodinger, wcjordan, mycrobe and somavara March 5, 2020 16:34

wcjordan approved these changes Mar 9, 2020

View reviewed changes

boxfoot force-pushed the master branch from ab05c3e to 96782b9 Compare March 9, 2020 15:59

Remove hasteResolver in favor of standard esm relative imports

12cb830

boxfoot force-pushed the master branch from 96782b9 to 12cb830 Compare March 9, 2020 16:06

boxfoot mentioned this pull request Mar 10, 2020

Lint and format with eslint and prettier #520

Merged

5 tasks

wcjordan approved these changes Mar 17, 2020

View reviewed changes

pradeepnschrodinger approved these changes Mar 23, 2020

View reviewed changes

pradeepnschrodinger merged commit 8677056 into schrodinger:master Sep 16, 2020

pradeepnschrodinger mentioned this pull request Nov 25, 2021

Clean up - imports (beta) #449

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove hasteResolver in favor of standard esm relative imports [Fix #513] #519

Remove hasteResolver in favor of standard esm relative imports [Fix #513] #519

boxfoot commented Mar 5, 2020

wcjordan commented Mar 9, 2020

wcjordan left a comment

wcjordan Mar 9, 2020

wcjordan Mar 9, 2020

wcjordan Mar 9, 2020

boxfoot commented Mar 9, 2020

wcjordan commented Mar 9, 2020

wcjordan commented Mar 17, 2020

pradeepnschrodinger left a comment

wcjordan commented Mar 23, 2020

boxfoot commented Mar 23, 2020

wcjordan commented Mar 23, 2020 •

edited

Loading

wcjordan commented May 11, 2020

wcjordan commented Jul 21, 2020

pradeepnschrodinger commented Aug 11, 2020

pradeepnschrodinger commented Sep 22, 2020

boxfoot commented Sep 22, 2020

Remove hasteResolver in favor of standard esm relative imports [Fix #513] #519

Remove hasteResolver in favor of standard esm relative imports [Fix #513] #519

Conversation

boxfoot commented Mar 5, 2020

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

wcjordan commented Mar 9, 2020

wcjordan left a comment

Choose a reason for hiding this comment

wcjordan Mar 9, 2020

Choose a reason for hiding this comment

wcjordan Mar 9, 2020

Choose a reason for hiding this comment

wcjordan Mar 9, 2020

Choose a reason for hiding this comment

boxfoot commented Mar 9, 2020

wcjordan commented Mar 9, 2020

wcjordan commented Mar 17, 2020

pradeepnschrodinger left a comment

Choose a reason for hiding this comment

wcjordan commented Mar 23, 2020

boxfoot commented Mar 23, 2020

wcjordan commented Mar 23, 2020 • edited Loading

wcjordan commented May 11, 2020

wcjordan commented Jul 21, 2020

pradeepnschrodinger commented Aug 11, 2020

pradeepnschrodinger commented Sep 22, 2020

boxfoot commented Sep 22, 2020

wcjordan commented Mar 23, 2020 •

edited

Loading