-
-
Notifications
You must be signed in to change notification settings - Fork 586
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
Support TypeScript .js
imports
#480
Conversation
Thanks for the PR. Unfortunately tests for all fixes a s features are a hard requirement. We won't be able to accept your PR until tests for this are added. |
All tests pass on my system: @shellscape Could let know what test is failing? Circle CI won't let me see their test output without giving them write access to all my repos which is pretty disgusting - so I won't be doing that. I checked the "Contributing" section of your README.md but apparently there's an extra step I'm missing. Thanks! |
Looks like a linting error is causing the CI failure:
|
Great thanks for the quick response! I now have it happening on my side too. Looking into it |
The last commit has a bunch of fixes that aren't related to #479. Happens 🤷 |
@shellscape / @NotWoods (or anyone) is there anything holding up this PR from being merged? If the concept is controversial or something let me know. |
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.
LGTM but I'd like to see @shellscape weigh in first
Ok sounds good, I just wanted to check it was live. No rush. Thanks! |
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.
We rarely get PRs that touch on multiple plugins at once, so I suppose now is the time to add this info to CONTRIBUTING.md:
- Commits should follow https://www.conventionalcommits.org/en/v1.0.0/, and scope names are limited to plugin names, minus
@rollup/plugin-
- PRs which touch multiple plugins are generally discouraged. For those that must be, each plugin should have it's own individual commits. Commits shouldn't touch more than one plugin
- PRs which touch multiple plugins must be merged with a regular "Merge Commit," rather than a squash merge.
We work this way to generate accurate per-plugin change logs and versioning. This is contrary to the way that other projects like Babel manage plugins in their monorepo, as our plugins are individually published (rather than all plugins published on the same version, sharing changelogs)
It's great that you fixed a bunch of files in other plugins. That's certainly welcome. Unfortunately, the commits in the branch don't match something that we can use for changelogs for the plugins. Because of that, we'll need you to break this into separate PRs. I would recommend branching fresh from master and cherry picking changes to individual plugins, then creating a PR on those branches. e.g. one branch for the changes in node-resolve, one for typescript, one for url, etc.
.eslintrc.js
Outdated
@@ -1,25 +1,35 @@ | |||
module.exports = { | |||
extends: ['rollup', 'plugin:import/typescript'], | |||
extends: ['rollup'], |
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 is this change necessary? I'm using virtually the same config in several projects without issue.
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 factored out all TS-related ESLint stuff to one "override" block that only operates on .ts/.tsx. So the "extends" has import/typescript in there instead of the general JS config
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.
If you want it in both JS and TS I could set that, but it feels like it belongs in only 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.
I'd prefer to revert this change and take a look at the merits of modifying it in an issue, followed by a repo-level commit if we want to move in that direction. The reason being is that this will effect all open PRs and development for all plugins moving forward, and I'm not ready to commit to that big a change.
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.
Yeah no worries. It just seemed to make sense at the time but I have no preference. Reverted the change
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.
my apologies if it was unclear, but I was speaking to all of the changes in the file. We're totally open to discussing a refactor of the config in another pipeline.
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.
Oh - that was not clear yeah.
I brought these changes in because the build fails without them.
From @NotWoods
Looks like a linting error is causing the CI failure:
> @rollup/plugin-node-resolve@8.1.0 lint:js /home/circleci/project/packages/node-resolve > eslint --fix --cache src test types --ext .js,.ts /home/circleci/project/packages/node-resolve/test/fixtures/extensions/main.js 0:0 error Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser. The file does not match your project config: packages/node-resolve/test/fixtures/extensions/main.js. The file must be included in at least one of the projects provided ✖ 1 problem (1 error, 0 warnings)
This is how I limit the TS parser to only TS files (typescript-eslint/typescript-eslint#1928). I'll look into doing it another way tomorrow
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 see, sorry that I missed that. You can place an overriding .eslintrc.js
file in that test directory to modify that behavior. we've done that often in the repo and is perfectly acceptable. If there are too many errors to contend with, you can add that to the .eslintignore
file. Fixtures always get wiggle room.
For "conventional commits" are you asking me to squash the work into one commit called
This PR only wants to touch I'll rebase with a squashed commit following your naming convention and remove the commit that does ESLint fixes. |
Good deal. I addressed all of the changed files so as not to minimize any work that went into them. |
bb3b327
to
5fe8ce5
Compare
Hmm @shellscape do you know anything about the broken lockfile? There was a conflict so I deleted the lockfile and reran |
Give |
Oh yeah I should have mentioned I did try that. I'll keep poking around |
It seems like you're working on dynamic-import-vars right now which might be related...
|
Might have to merge from upstream/master maybe. |
✨✨✨ Ok @shellscape if you can reply to the thread/review you opened that's all |
That's seems odd because I actually have to commit with But if it works for you that's what's important. Anyway I fixed it (on my end; we'll see if GitHub likes it) by moving TS files to their own folder - it seems ESLint will operate on entire folders so any JS files will be sucked into the TS mess. |
Oops. |
OK, I'll clone your fork and branch to take a look. |
My bad about the tests |
⭐ Looks good - I'll squash the commits |
Thanks for working with us on this one. |
Thanks both of you for helping to get it merged in :) |
Closes #479
Rollup Plugin Name:
node-resolve
This PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
#479
Description
As described in #479 it's common for a TS project to use '.js' to refer to local '.ts' or '.tsx' files. This adds support for that.
Previously/Currently you need to import as either
'./file'
(see quote in #479 😉) or'./file.ts'
. Both don't really work:The fix:
I haven't added tests because it's only 5 lines of code and the "algorithm" is just "Is a TS/TSX file importing a JS file and
{ extensions: [] }
has TS and/or TSX in it? Then add toimportSpecifierList
as a potential resolution".Note that
@rollup/plugin-typescript
works out of the box for this already, but I think it'd be good to not need to require the full TS compiler just to support this - VSCode already lints the whole project, I don't need a compile, I just need types removed via Babel (so so much faster)