(alias) Breaking feature: built-in resolving algorithm is replaced in favor of Rollup's `this.resolve()` #34

Acionyx · 2019-11-10T19:43:08Z

Rollup Plugin Name: alias

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

Related issues

#14
#12

Description

So, after short discussion with @lukastaegert we agreed that at least this feature should be discussed with community.
This feature enables default resolving behaviour for Alias plugin.
The reasons:

Alias plugin is made for 'aliasing', not for 'resolving';
Before customResolver was implemented, it was impossible to create some complex Rollup configurations with Alias plugin.
After customResolver was implemented, it seems logical to remove any hidden resolving logic.

Overall simplicity and transparent behavior are key for good plugin as i think.

According to pnpm actual version and included packages

Allow to use customResolver instead of built-in resolving algorithm in

customResolver option

customResolver description

@@ BREAKING @@ And replaced by Rollup's context function `this.resolve()`

Acionyx · 2019-11-16T17:14:43Z

Hi @shellscape, may you invite others to review this PR?
I would like to make final fixes, changes to Readme, etc, if the main idea of this PR is accepted and has sense.

shellscape · 2019-11-16T17:34:46Z

Done! I've added @Andarist and @lukastaegert to review.

packages/alias/src/index.js

Acionyx · 2019-11-16T20:38:56Z

Done! I've added @Andarist and @lukastaegert to review.

Thanks!

packages/alias/src/index.js

packages/alias/test/test.js

lukastaegert

The production code appears good to me as it is, but I have profound suggestions for how to improve the tests and make them more realistic. Also, you should run pnpm run lint, your changes cause a lot of linter warnings.

packages/alias/test/test.js

lukastaegert · 2019-11-17T08:12:30Z

packages/alias/test/test.js

@@ -144,79 +114,26 @@ test('Leaves entry file untouched if matches alias', (t) => {
  t.is(resolved, null);
 });

-test('Test for the resolve property', (t) => {
-  const result = alias({
-    resolve: ['.js', '.jsx'],


As I understand, this flag has been removed, but it is still referenced in the Readme. It would be nice to update the readme to

remove this

mention that this plugin will use the resolver plugins specified for Rollup, and that if you rely on Node specific features, you probably want rollup-plugin-node-resolve in your setup

Done. Please find changes in latest version.

lukastaegert · 2019-11-17T09:30:53Z

packages/alias/test/test.js

  });

  const resolved = result.resolveId('resolve', '/src/import.js');

  t.is(resolved, 'i/am/a/file');
 });

-test('i/am/a/local/file', (t) => {


Again, it would be interesting to reintroduce these as integration tests involving Rollup and possibly rollup-plugin-node-resolve. Actually I think it would be nice to run ALL tests through Rollup, this would make the tests much more realistic and we could also get rid of the customResolve: x => x that needed to be introduced into many tests to keep the structure. Here is a helper that would work:

// "plugins" is an array of plugins for Rollup, they should include "alias" // "tests" is an array of pairs [importee, importer] function resolveWithRollup(plugins, tests) { return new Promise((resolve, reject) => { rollup({ input: 'dummy-input', plugins: [ { name: 'test-plugin', buildStart() { resolve( // The buildStart hook is the first to have access to this.resolve // We map the tests to an array of resulting ids Promise.all( tests.map(([importee, importer]) => this.resolve(importee, importer).then(({ id }) => id) ) ) ); }, resolveId(id) { if (id === 'dummy-input') return id; return null; }, load(id) { if (id === 'dummy-input') return 'console.log("test");'; return null; } }, ...plugins ] // if Rollup throws an error, this should reject the test }).catch(reject); }); } // convenience function for just injecting the alias plugin function resolveAliasWithRollup(aliasOptions, tests) { return resolveWithRollup([alias(aliasOptions)], tests); }

Here is an example test, rewritten using the helper:

test('Simple aliasing (array)', (t) => resolveAliasWithRollup( { entries: [ { find: 'foo', replacement: 'bar' }, { find: 'pony', replacement: 'paradise' }, { find: './local', replacement: 'global' } ] }, [ ['foo', '/src/importer.js'], ['pony', '/src/importer.js'], ['./local', '/src/importer.js'] ] ).then((result) => t.deepEqual(result, ['bar', 'paradise', 'global'])));

For tests involving the alias plugin and other plugins, the first helper could be used.

Thanks for detailed example. I was thinking in same way, but stopped at dilemma: to include rollup-plugin-node-resolve or not. My point is that it is not a good idea to include 3rd party plugin into alias plugin tests as it is not reliable in long-term. But right now i do not see any better option to keep tests coverage at good level without doing it.

Probably it would be better to include rollup-plugin-node-resolve after #32 is merged. However it does not block other changes.

Done. Please find changes in latest version.

My point is that it is not a good idea to include 3rd party plugin into alias plugin tests as it is not reliable in long-term

It will make the test slightly less stable, but as rollup-plugin-node-resolve is an important pillar of the Rollup ecosystem, it will basically improve consistency of the wider ecosystem, which I would love very much.

Probably it would be better to include rollup-plugin-node-resolve after #32 is merged

That would definitely make it easier, but I am not sure how long this will take, this is not a trivial move. If you reference the external plugin for now, we should just not forget to change the reference once #32 is merged. Should not be a big change, but this does not need to be coupled.

# Conflicts: # packages/alias/test/test.js

Tests: every test is done by invoking Rollup instance; Plugin: fixed according to linter warnings; Readme: updated.

Andarist · 2019-11-17T21:05:34Z

packages/alias/README.md

@@ -52,7 +54,7 @@ module.exports = {
    dir: 'output',
    format: 'cjs'
  },
-  plugins: [alias({ resolve: ['.jsx', '.js'] })]
+  plugins: [alias()]


would be good to include example usage here

Andarist · 2019-11-17T21:06:58Z

packages/alias/test/test.js

@@ -10,6 +10,47 @@ import alias from '../dist';
 const normalizePath = (pathToNormalize) => slash(pathToNormalize.replace(/^([A-Z]:)/, ''));
 const DIRNAME = normalizePath(__dirname);

+// "plugins" is an array of plugins for Rollup, they should include "alias"


could you convert those 2 lines to a JSDOC? that way this function would get nice hints in IDEs

packages/alias/test/test.js

Andarist · 2019-11-17T21:12:08Z

packages/alias/test/test.js

+        { find: './local', replacement: 'global' }
+      ]
+    },
+    [['foo', '/src/importer.js'], ['pony', '/src/importer.js'], ['./local', '/src/importer.js']]


Personally would prefer for a single test to be an object ({ requester, importSource } or similar) rather than a tuple, but if you prefer tuples I won't insist on this one.

Agreed, makes the test probably a bit more readable. Maybe stick with the terminology used in Rollup's docs: https://rollupjs.org/guide/en/#resolveid which would be {source, importer}

Acionyx · 2019-11-24T22:03:44Z

Hi, @Andarist, @lukastaegert, please find new changes here according all mentioned points.

shellscape · 2019-12-03T00:04:24Z

@Acionyx we won't let this one fall off the radar. sometimes it takes them a few days to circle back.

shellscape · 2019-12-09T16:09:09Z

@Andarist @lukastaegert please put your peepers back on this one when you have the chance.

shellscape · 2019-12-15T20:09:20Z

@Acionyx we've got an approval to merge. We'll need you to update pnpm-lock.yaml however. With our monorepo setup that sometimes gets too far out of sync and can't be merged:

rm pnpm-lock.yaml
git merge upstream/master
pnpm install --force

Should get it back to mergable.

shellscape · 2019-12-16T16:33:01Z

Thanks to everyone for working on this one. Please have patience for a new major version. We're going to try to get our publishing tools setup to automate this.

Acionyx · 2019-12-17T21:59:42Z

Hi @shellscape, thanks for catching up with this and helping with conflicts.

Thanks all, hope those changes will be warmly met by community :)

Acionyx and others added 11 commits October 30, 2019 23:39

ignore Intellij IDEA dir

0904c53

updated pnpm-lock.yaml

db29eec

According to pnpm actual version and included packages

@rollup/plugin-alias: customResolver option

1fdd9a0

Allow to use customResolver instead of built-in resolving algorithm in

@rollup/plugin-alias: updated readme

bac27d5

customResolver option

(alias) updated README

7b423a6

customResolver description

docs: rework custom resolver options and section

c809ee5

Merge branch 'master' of github.com:rollup/plugins

d1bed17

(alias) customResolver bugfix

9c58dd9

Merge branch 'master' of github.com:rollup/plugins

c60adf9

feat(alias): Built-in resolving algorithm is removed

3822810

@@ BREAKING @@ And replaced by Rollup's context function `this.resolve()`

Merge branch 'master' of github.com:rollup/plugins

5cfdcba

shellscape requested review from Andarist and lukastaegert November 16, 2019 17:34

Andarist reviewed Nov 16, 2019

View reviewed changes

packages/alias/src/index.js Outdated Show resolved Hide resolved

Andarist reviewed Nov 16, 2019

View reviewed changes

packages/alias/src/index.js Show resolved Hide resolved

Andarist reviewed Nov 16, 2019

View reviewed changes

packages/alias/test/test.js Show resolved Hide resolved

lukastaegert requested changes Nov 17, 2019

View reviewed changes

Acionyx added 2 commits November 17, 2019 22:21

Merge branch 'master' of github.com:rollup/plugins

25f6242

# Conflicts: # packages/alias/test/test.js

(alias) Fixes according to PR discussion

f8dab3e

Tests: every test is done by invoking Rollup instance; Plugin: fixed according to linter warnings; Readme: updated.

Acionyx requested review from Andarist and lukastaegert November 17, 2019 20:56

Acionyx changed the title ~~WIP: (alias) Breaking feature: built-in resolving algorithm is replaced in favor of Rollup's this.resolve()~~ (alias) Breaking feature: built-in resolving algorithm is replaced in favor of Rollup's this.resolve() Nov 17, 2019

Andarist reviewed Nov 17, 2019

View reviewed changes

packages/alias/test/test.js Show resolved Hide resolved

Andarist reviewed Nov 17, 2019

View reviewed changes

Merge branch 'master' of github.com:rollup/plugins

c779d1f

(alias) fixes according to review

e602e90

Acionyx requested a review from Andarist November 24, 2019 22:02

shellscape mentioned this pull request Dec 9, 2019

dedupe leads to errors in a monorepo with hoisted dependencies #86

Closed

Andarist approved these changes Dec 14, 2019

View reviewed changes

lukastaegert approved these changes Dec 16, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master'

1fc2c22

shellscape added t³ ✨ enhancement v¹ ⋅ major 🔌 plugin-alias labels Dec 16, 2019

shellscape merged commit 46c75f4 into rollup:master Dec 16, 2019

This was referenced Jan 3, 2020

use external #12

Closed

Don't add file extensions automatically #14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(alias) Breaking feature: built-in resolving algorithm is replaced in favor of Rollup's `this.resolve()` #34

(alias) Breaking feature: built-in resolving algorithm is replaced in favor of Rollup's `this.resolve()` #34

Acionyx commented Nov 10, 2019 •

edited

Loading

Acionyx commented Nov 16, 2019

shellscape commented Nov 16, 2019

Acionyx commented Nov 16, 2019

lukastaegert left a comment

lukastaegert Nov 17, 2019

Acionyx Nov 17, 2019

lukastaegert Nov 17, 2019

Acionyx Nov 17, 2019

Acionyx Nov 17, 2019

Acionyx Nov 17, 2019

lukastaegert Nov 18, 2019

Andarist Nov 17, 2019

Andarist Nov 17, 2019

Andarist Nov 17, 2019

lukastaegert Nov 18, 2019

Acionyx commented Nov 24, 2019

shellscape commented Dec 3, 2019

shellscape commented Dec 9, 2019

shellscape commented Dec 15, 2019

shellscape commented Dec 16, 2019

Acionyx commented Dec 17, 2019 •

edited

Loading

(alias) Breaking feature: built-in resolving algorithm is replaced in favor of Rollup's this.resolve() #34

(alias) Breaking feature: built-in resolving algorithm is replaced in favor of Rollup's this.resolve() #34

Conversation

Acionyx commented Nov 10, 2019 • edited Loading

Related issues

Description

Acionyx commented Nov 16, 2019

shellscape commented Nov 16, 2019

Acionyx commented Nov 16, 2019

lukastaegert 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Acionyx commented Nov 24, 2019

shellscape commented Dec 3, 2019

shellscape commented Dec 9, 2019

shellscape commented Dec 15, 2019

shellscape commented Dec 16, 2019

Acionyx commented Dec 17, 2019 • edited Loading

(alias) Breaking feature: built-in resolving algorithm is replaced in favor of Rollup's `this.resolve()` #34

(alias) Breaking feature: built-in resolving algorithm is replaced in favor of Rollup's `this.resolve()` #34

Acionyx commented Nov 10, 2019 •

edited

Loading

Acionyx commented Dec 17, 2019 •

edited

Loading