-
Notifications
You must be signed in to change notification settings - Fork 50
Fix imports/exports from local sibling modules #639
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like how it does this. Since they are re-exported from the main module they should be there instead / as well. My previous attempt added the files to shortenedFiles which did the right thing in that regards but it messed up other stuff. That is probably a separate issue and would change a lot of the way things are generated whereas this change seems relatively isolated.
f601d46 to
20f0939
Compare
|
There might be something not right here. It's getting confused between es and lib. For example: |
|
Ok I think that's fixed now. The problem was that it was adding a ModuleAliases from es to lib which was overwriting and/or higher priority than the normal module scope. |
|
Interestingly this fixes another existing bug which is the opposite to the problem I had above.
import type { IconDefinition } from '@ant-design/icons-svg/lib/types';Which used to incorrectly resolve to |
44ce77e to
d0d3331
Compare
|
Ughhh so the I have no idea why fixing that triggered a whole lot of changes in the react-integration-tests. Seems like it is all new stuff which is good in a way but the |
| "main": "lib/index.js", | ||
| "module": "es/index.js", | ||
| "unpkg": "dist/antd.min.js", | ||
| "typings": "lib/index.d.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The real package uses es typings.
| "com.olvind" %%% "scalablytyped-runtime" % "2.4.2", | ||
| "org.scalablytyped" %%% "react" % "16.9.2-eebc56", | ||
| "org.scalablytyped" %%% "react" % "16.9.2-d4e96d", | ||
| "org.scalablytyped" %%% "react-bootstrap" % "0.32-edeae4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this coming from?
| libraryDependencies ++= Seq( | ||
| "com.github.japgolly.scalajs-react" %%% "core" % "2.1.1", | ||
| "com.olvind" %%% "scalablytyped-runtime" % "2.4.2", | ||
| "org.scalablytyped" %%% "react" % "16.9.2-eebc56", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did this go?
| japgolly.scalajs.react.facade.React.Component[ButtonGroupProps & js.Object, js.Object] | ||
| ] { | ||
| /* import warning: RemoveDifficultInheritance.summarizeChanges | ||
| - Dropped / * import warning: transforms.QualifyReferences#resolveTypeRef many Couldn't qualify React.HTMLProps<ButtonGroup> * / any */ trait ButtonGroupProps extends StObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why dropped all of a sudden?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the error look whack? Seems like a double error or something.
|
The |
Fixes #638
Consider the following structure:
Where
package.jsonhas:and
b/index.d.tshas:Currently this would not generate any of the types from
b.It seems it was intentional to only consider files from
typesandtypings, or themodulebut this causes lookups from sibling modules to fail.