Support TypeScript `.js` imports #480

nettybun · 2020-06-30T16:50:35Z

Closes #479

Rollup Plugin Name: `node-resolve`

This PR contains:

Are tests included?

yes (bugfixes and features will not be merged without tests)
no

Breaking Changes?

yes (breaking changes will not be merged unless absolutely necessary)
no

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 to importSpecifierList 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)

shellscape · 2020-07-01T11:53:32Z

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.

nettybun · 2020-07-01T20:44:12Z

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!

NotWoods · 2020-07-01T20:57:26Z

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)

nettybun · 2020-07-01T21:09:13Z

Great thanks for the quick response! I now have it happening on my side too. Looking into it

nettybun · 2020-07-01T21:34:46Z

The last commit has a bunch of fixes that aren't related to #479. Happens 🤷

nettybun · 2020-07-03T22:47:54Z

@shellscape / @NotWoods (or anyone) is there anything holding up this PR from being merged? If the concept is controversial or something let me know.

NotWoods

LGTM but I'd like to see @shellscape weigh in first

nettybun · 2020-07-03T23:07:15Z

Ok sounds good, I just wanted to check it was live. No rush. Thanks!

shellscape

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.

shellscape · 2020-07-06T16:10:56Z

.eslintrc.js

@@ -1,25 +1,35 @@
 module.exports = {
-  extends: ['rollup', 'plugin:import/typescript'],
+  extends: ['rollup'],


why is this change necessary? I'm using virtually the same config in several projects without issue.

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

If you want it in both JS and TS I could set that, but it feels like it belongs in only TS?

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.

Yeah no worries. It just seemed to make sense at the time but I have no preference. Reverted the change

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.

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

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.

nettybun · 2020-07-06T16:48:47Z

For "conventional commits" are you asking me to squash the work into one commit called fix(node-resolve): support .js extension #479? I know your repo uses conventional commits but really it feels like it should be up to you as a merger to decide what commits you want. If this a fix or a feat?

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

This PR only wants to touch node-resolve. The very last commit that runs ESLint to autofix problems that somehow snuck into your repo to fix semicolons/quotes isn't part of the idea behind the PR. I'm just sweeping up some dust on the way out. If you don't want your own ESLint fixes that's fine - I'll put the dust back and remove the commit. You can run the linter yourself and merge it into master :) I think it makes more sense than opening 4-5 PRs for a semicolon fix

I'll rebase with a squashed commit following your naming convention and remove the commit that does ESLint fixes.

shellscape · 2020-07-06T17:01:43Z

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.

nettybun · 2020-07-06T17:20:28Z

Hmm @shellscape do you know anything about the broken lockfile? There was a conflict so I deleted the lockfile and reran pnpm i. CircleCI is fine with it but your GitHub checks fail :/

shellscape · 2020-07-06T17:35:24Z

Give pnpm install --no-frozen-lockfile a try. If that doesn't resolve it I'll make some time to fix it.

nettybun · 2020-07-06T17:41:23Z

Oh yeah I should have mentioned I did try that. I'll keep poking around

nettybun · 2020-07-06T17:50:52Z

It seems like you're working on dynamic-import-vars right now which might be related...

ERROR  Cannot install with "frozen-lockfile" because pnpm-lock.yaml is not up-to-date with packages\dynamic-import-vars\package.json

shellscape · 2020-07-06T19:27:13Z

Might have to merge from upstream/master maybe.

nettybun · 2020-07-06T23:58:25Z

✨✨✨

Ok @shellscape if you can reply to the thread/review you opened that's all

nettybun · 2020-07-07T19:40:21Z

why is this change necessary? I'm using virtually the same config in several projects without issue.

That's seems odd because I actually have to commit with git commit --no-verify to bypass ESLint failing on .eslinrc.js with the old config:

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.

nettybun · 2020-07-07T19:40:37Z

Oops.

shellscape · 2020-07-07T19:43:44Z

OK, I'll clone your fork and branch to take a look.

nettybun · 2020-07-07T19:44:59Z

My bad about the tests

nettybun · 2020-07-07T19:48:37Z

⭐ Looks good - I'll squash the commits

shellscape · 2020-07-07T20:07:53Z

Thanks for working with us on this one.

nettybun · 2020-07-07T20:16:49Z

Thanks both of you for helping to get it merged in :)

nettybun added a commit to nettybun/plugins that referenced this pull request Jul 1, 2020

Add test for TS imports that use JS extensions (rollup#480)

c1024ec

NotWoods approved these changes Jul 3, 2020

View reviewed changes

shellscape requested changes Jul 6, 2020

View reviewed changes

nettybun force-pushed the patch-1 branch 2 times, most recently from bb3b327 to 5fe8ce5 Compare July 6, 2020 17:17

nettybun force-pushed the patch-1 branch from e2d0991 to 857b009 Compare July 6, 2020 17:47

nettybun force-pushed the patch-1 branch from 857b009 to 822e833 Compare July 6, 2020 17:51

nettybun force-pushed the patch-1 branch from 822e833 to c799c54 Compare July 6, 2020 23:52

nettybun force-pushed the patch-1 branch from c799c54 to 440d9e1 Compare July 7, 2020 00:56

nettybun requested a review from shellscape July 7, 2020 00:58

nettybun closed this Jul 7, 2020

nettybun reopened this Jul 7, 2020

fix(node-resolve): support .js imports in TypeScript (rollup#479)

3421151

nettybun force-pushed the patch-1 branch from d19632c to 3421151 Compare July 7, 2020 19:49

shellscape merged commit 56b3725 into rollup:master Jul 7, 2020

nettybun deleted the patch-1 branch July 7, 2020 20:14

LarsDenBakker pushed a commit to LarsDenBakker/plugins that referenced this pull request Sep 12, 2020

feat(node-resolve): support .js imports in TypeScript (rollup#480)

f7abdb8

bhovhannes mentioned this pull request Jun 30, 2021

Cannot bundle a TS file, which imports JS file from a package using NodeJS conditional exports #911

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support TypeScript `.js` imports #480

Support TypeScript `.js` imports #480

nettybun commented Jun 30, 2020 •

edited

shellscape commented Jul 1, 2020

nettybun commented Jul 1, 2020

NotWoods commented Jul 1, 2020

nettybun commented Jul 1, 2020

nettybun commented Jul 1, 2020

nettybun commented Jul 3, 2020 •

edited

NotWoods left a comment

nettybun commented Jul 3, 2020

shellscape left a comment

shellscape Jul 6, 2020

nettybun Jul 6, 2020

nettybun Jul 6, 2020

shellscape Jul 7, 2020

nettybun Jul 7, 2020

shellscape Jul 7, 2020

nettybun Jul 7, 2020

shellscape Jul 7, 2020

nettybun commented Jul 6, 2020 •

edited

shellscape commented Jul 6, 2020

nettybun commented Jul 6, 2020

shellscape commented Jul 6, 2020

nettybun commented Jul 6, 2020

nettybun commented Jul 6, 2020

shellscape commented Jul 6, 2020

nettybun commented Jul 6, 2020

nettybun commented Jul 7, 2020

nettybun commented Jul 7, 2020

shellscape commented Jul 7, 2020

nettybun commented Jul 7, 2020

nettybun commented Jul 7, 2020 •

edited

shellscape commented Jul 7, 2020

nettybun commented Jul 7, 2020

Support TypeScript .js imports #480

Support TypeScript .js imports #480

Conversation

nettybun commented Jun 30, 2020 • edited

Rollup Plugin Name: node-resolve

Description

shellscape commented Jul 1, 2020

nettybun commented Jul 1, 2020

NotWoods commented Jul 1, 2020

nettybun commented Jul 1, 2020

nettybun commented Jul 1, 2020

nettybun commented Jul 3, 2020 • edited

NotWoods left a comment

Choose a reason for hiding this comment

nettybun commented Jul 3, 2020

shellscape left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nettybun commented Jul 6, 2020 • edited

shellscape commented Jul 6, 2020

nettybun commented Jul 6, 2020

shellscape commented Jul 6, 2020

nettybun commented Jul 6, 2020

nettybun commented Jul 6, 2020

shellscape commented Jul 6, 2020

nettybun commented Jul 6, 2020

nettybun commented Jul 7, 2020

nettybun commented Jul 7, 2020

shellscape commented Jul 7, 2020

nettybun commented Jul 7, 2020

nettybun commented Jul 7, 2020 • edited

shellscape commented Jul 7, 2020

nettybun commented Jul 7, 2020

Support TypeScript `.js` imports #480

Support TypeScript `.js` imports #480

nettybun commented Jun 30, 2020 •

edited

Rollup Plugin Name: `node-resolve`

nettybun commented Jul 3, 2020 •

edited

nettybun commented Jul 6, 2020 •

edited

nettybun commented Jul 7, 2020 •

edited