add the ability to rewrite modules in-place #1922

kellyselden · 2018-01-28T22:49:36Z

Closes #1878.

This turned out to be remarkably simpler once @guybedford laid the foundational work with dynamic imports and chunking. I essentially tapped into the dynamic imports system and made every import behave like a dynamic import.

To try this out, all you have to do is add the experimentalPreserveModules to your existing experimentalCodeSplitting setup. Then, the directory tree should be preserved in your output dir.

kellyselden · 2018-01-29T00:27:39Z

Turns out the the folders are not preserved: "app/index.js" => "index.js". I will work on that next.

kellyselden · 2018-01-29T02:04:32Z

Added another option to enable original directory structure.

guybedford

I've included some feedback here, let me know your thoughts.

guybedford · 2018-02-11T10:52:43Z

src/Graph.ts

+			curEntry.isEntryPoint = true;
+			curEntryHash = randomUint8Array(10);
+			visit(curEntry);
+		}


It seems like this is done to ensure that no chunking happens right?

I think it can be done more easily by altering the visit function itself to do something like:

if (!preserveModules) Uint8ArrayXor(module.entryPointsHash, curEntryHash); else if (isZero(module.entryPointsHash)) module.entryPointsHash = randomUint8Array(10);

then I think that would be all there is too it without any extra loops or need to track modules.

Perfect! It got a lot simpler with this change.

guybedford · 2018-02-11T10:53:24Z

src/Graph.ts

+				preservedModules.forEach(module => {
+					if (entryModules.indexOf(module) === -1)
+						entryModules.push(module);
+				});


This is just "all modules" right? If so it should be possible to generate from Object.values(this.graph) + this.externals?

removed with your other suggested change.

guybedford · 2018-02-11T10:55:23Z

src/Graph.ts

@@ -396,7 +421,7 @@ export default class Graph {
 				chunkList.forEach(chunk => {
 					// generate the imports and exports for the output chunk file
 					if (chunk.entryModule) {
-						const entryName = generateChunkName(chunk.entryModule.id, chunkNames, true);
+						const entryName = generateChunkName(chunk.entryModule.id, chunkNames, true, inputRelativeDir);


Is this a mandatory option then? Is there not a way to deduce the lowest base by default if this is not provided?

I was able to rely on the package commondir to calculate given the entry modules. It is used in a subsequent feature request here to make it something other than common base path, but I will move the option into that branch instead.

Since commondir is not widely maintained and a small piece of code, do you think we could just inline the implementation?

I could do that. Are you sure I should just copy paste? I would argue that we depend on it until we need to fix something, then copy. @lukastaegert care to tie-break?

I must admit I did not read everything here. Looking at commondir, it relies on nodes path.resolve which we do not have available in a browser build—even though we have our own polyfill for that (damn, we REALLY need a test for this. You should see a warning about "path" being external when building rollup with this setup—this is always BAD). Thus I fear this needs to go into our own code base somehow.

However I am happy for every bit of functionality we can extract from Graphs.ts and put into other files and for me, this is a clear candidate for the "utils" folder.

ported into the utils folder.

guybedford · 2018-02-12T09:35:48Z

It's pretty awesome how small this feature has got. And it definitely preserves the original pathnames ok with directory creation etc?

kellyselden · 2018-02-13T01:38:31Z

From my testing, yes. It does get weird if your entry file imports from '../../../../../x', then your output dir will look pretty strange, but it should still work and be "correct".

guybedford

Looking good.

@lukastaegert would be interested to hear your thoughts here as well.

guybedford · 2018-02-15T10:51:38Z

src/utils/commondir.ts

@@ -0,0 +1,21 @@
+// ported from https://github.com/substack/node-commondir
+export default function commondir (relfiles: string[]) {
+    var files = relfiles;


Perhaps just name the argument files/

guybedford · 2018-02-15T10:51:50Z

src/utils/commondir.ts

+export default function commondir (relfiles: string[]) {
+    var files = relfiles;
+
+    var res = files.slice(1).reduce(function (ps, file) {


const and arrow function can be used.

guybedford · 2018-02-15T10:52:04Z

src/utils/commondir.ts

+    var res = files.slice(1).reduce(function (ps, file) {
+        if (!file.match(/^([A-Za-z]:)?\/|\\/)) {
+            throw new Error('relative path without a basedir');
+        }


If we know that our inputs will pass this, it may be possible to remove this check.

guybedford · 2018-02-15T10:52:16Z

src/utils/commondir.ts

+            throw new Error('relative path without a basedir');
+        }
+
+        var xs = file.split(/\/+|\\+/);


guybedford · 2018-02-15T10:53:02Z

src/utils/commondir.ts

+        for (
+            var i = 0;
+            ps[i] === xs[i] && i < Math.min(ps.length, xs.length);
+            i++


Tailing semicolon.

compiler didn't like that

guybedford · 2018-02-15T10:54:33Z

src/Graph.ts

+		name = path.relative(inputRelativeDir, id).replace(/\\/g, '/');
+	} else {
+		name = path.basename(id);
+	}


Check indentation here.

One thing I observed that I think we should definitely improve is the fact that if a module imports something from outside the common directory of the entry points (i.e. if the entry point imports something that starts with ../), new files and folders are created outside the intended target directory. This can definitely leave a mess on an unsuspecting user's system. In my opinion there are two possible solutions

throw an error if something like this would happen (probably not ideal), or

find the common root directory not among the entry points but among the generated chunk file names.

To achieve the latter, we could do the following:

use generateChunkName only for the non-path-preserving case (and remove the last argument)

if we preserve paths/modules, we use the completed chunks object to build a new object with correct paths (possibly extract a helper method here)

This is just a rough idea, some fine-tuning may be necessary.

lukastaegert · 2018-02-15T11:46:27Z

Sure thing. Was a little busy the last days to get my own PR in order. Will probably take a look at this evening or tomorrow. I think this could become on of the core features of 0.57.0!

lukastaegert · 2018-02-15T11:47:46Z

BTW some of the chunking form tests seem to be broken. And this should probably be rebased against the current master soon.

kellyselden · 2018-02-15T16:54:56Z

@lukastaegert I discovered a bug, so I added more test cases. I'm trying to diagnose it now.

kellyselden · 2018-02-16T05:32:27Z

@lukastaegert @guybedford Turns out it might not be a bug, but an understanding issue. It has to do with what's going on in \test\chunking-form\samples\chunking-reexport. The way it's collapsing reexports there is unexpected to me, and probably not desirable for this new feature. I would love some pointers.

guybedford · 2018-02-16T09:58:52Z

@kellyselden can you link me to the test case that is failing? I just re-ran the CI here and it seems nothing is breaking specifically. If I can follow the exact simplified scenario I may be able to suggest further.

kellyselden · 2018-02-16T15:59:11Z

@guybedford https://github.com/rollup/rollup/pull/1922/files#diff-c7e8e1a25cc0ed7f80f7f0df33f956c7R7
I skipped it because it appears to be working as you intended after comparing with \test\chunking-form\samples\chunking-reexport, I just don't understand why it is working that way, and if I should change it for this new feature. The _expected folder in my skipped test is what I would expect though.

kellyselden · 2018-03-05T19:21:40Z

@guybedford I added the test for export aliases. Since there are still two commits, I added the test to the first one, and export aliases worked as expected. Then I had to modify the test to remove the aliases once your commit was added on top. You probably already knew this, I just wanted to point it out in case you didn't.

lukastaegert

Sorry for not getting to this sooner. This works nicely now! As this is an experimental feature anyway, I'll merge this into the next release, we can always fledge out any last details later.

tunnckoCore · 2018-05-25T18:13:16Z

This option is still not documented on the site? It seems that i just should try it ;/

guybedford · 2018-05-26T12:03:30Z

I think we should document this now actually. A documentation PR is more than welcome!

kellyselden changed the title ~~add the ability to rewrite modules in-place~~ WIP: add the ability to rewrite modules in-place Jan 29, 2018

kellyselden force-pushed the preserve-modules-2 branch from 58a6c3b to 601f886 Compare January 29, 2018 02:00

kellyselden mentioned this pull request Jan 29, 2018

enabled testing directory structures in chunked mode #1924

Merged

kellyselden force-pushed the preserve-modules-2 branch 2 times, most recently from 08ed72d to c66aef5 Compare February 1, 2018 15:06

kellyselden mentioned this pull request Feb 4, 2018

add chunking failing test #1940

Closed

guybedford reviewed Feb 11, 2018

View reviewed changes

kellyselden force-pushed the preserve-modules-2 branch 2 times, most recently from 0a319a1 to d3006af Compare February 11, 2018 19:10

kellyselden mentioned this pull request Feb 11, 2018

WIP: preserve modules #1878

Closed

kellyselden force-pushed the preserve-modules-2 branch from d3006af to 086baab Compare February 11, 2018 22:05

kellyselden force-pushed the preserve-modules-2 branch 3 times, most recently from c9647d3 to 803d043 Compare February 15, 2018 06:39

guybedford reviewed Feb 15, 2018

View reviewed changes

kellyselden force-pushed the preserve-modules-2 branch 2 times, most recently from e061cc2 to 3849e93 Compare February 16, 2018 05:28

kellyselden changed the title ~~WIP: add the ability to rewrite modules in-place~~ add the ability to rewrite modules in-place Feb 16, 2018

kellyselden closed this Feb 16, 2018

kellyselden reopened this Feb 16, 2018

kellyselden force-pushed the preserve-modules-2 branch from 7481a1e to 5ede078 Compare March 5, 2018 19:08

Kelly Selden and others added 2 commits March 8, 2018 20:41

add the ability to rewrite modules in-place

0e4f3c5

entry point handling adjustments for tree shaking

f837ca7

kellyselden force-pushed the preserve-modules-2 branch from 5ede078 to f837ca7 Compare March 9, 2018 04:44

lukastaegert approved these changes Mar 14, 2018

View reviewed changes

lukastaegert merged commit f837ca7 into rollup:master Mar 14, 2018

lukastaegert added the Fixed pending release label Mar 14, 2018

kellyselden deleted the preserve-modules-2 branch March 14, 2018 17:09

lukastaegert removed the Fixed pending release label Mar 15, 2018

lastmjs mentioned this pull request Mar 19, 2018

Standalone transpilation rollup/rollup-plugin-commonjs#304

Closed

lukastaegert mentioned this pull request May 15, 2018

Question: Separation of dependency resolution? #1834

Closed

bennypowers mentioned this pull request Jul 30, 2018

Move to Standard Modules evilsoft/crocks#261

Closed

colinrotherham mentioned this pull request Mar 16, 2023

Compile ES modules using Rollup alphagov/govuk-frontend#3386

Merged

add the ability to rewrite modules in-place #1922

add the ability to rewrite modules in-place #1922

Conversation

kellyselden commented Jan 28, 2018 • edited Loading

kellyselden commented Jan 29, 2018

kellyselden commented Jan 29, 2018

guybedford 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

guybedford commented Feb 12, 2018

kellyselden commented Feb 13, 2018

guybedford 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukastaegert commented Feb 15, 2018

lukastaegert commented Feb 15, 2018

kellyselden commented Feb 15, 2018

kellyselden commented Feb 16, 2018

guybedford commented Feb 16, 2018

kellyselden commented Feb 16, 2018 • edited Loading

kellyselden commented Mar 5, 2018

lukastaegert left a comment • edited Loading

Choose a reason for hiding this comment

tunnckoCore commented May 25, 2018

guybedford commented May 26, 2018

kellyselden commented Jan 28, 2018 •

edited

Loading

kellyselden commented Feb 16, 2018 •

edited

Loading

lukastaegert left a comment •

edited

Loading