Skip to content
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

feat: add option to keep extensions for amd #4607

Merged
merged 9 commits into from Aug 31, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions cli/help.md
Expand Up @@ -23,6 +23,7 @@ Basic options:
--amd.autoId Generate the AMD ID based off the chunk name
--amd.basePath <prefix> Path to prepend to auto generated AMD ID
--amd.define <name> Function to use in place of `define`
--amd.keepExtension Add `.js` extension for generated chunks and local AMD modules
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should wrap at 80 characters, can we slightly shorten this? E.g. "Add .js extension in imports"

--assetFileNames <pattern> Name pattern for emitted assets
--banner <text> Code to insert at top of bundle (outside wrapper)
--chunkFileNames <pattern> Name pattern for emitted secondary chunks
Expand Down
1 change: 1 addition & 0 deletions docs/01-command-line-reference.md
Expand Up @@ -358,6 +358,7 @@ Many options have command line equivalents. In those cases, any arguments passed
--amd.autoId Generate the AMD ID based off the chunk name
--amd.basePath <prefix> Path to prepend to auto generated AMD ID
--amd.define <name> Function to use in place of `define`
--amd.keepExtension Add `.js` extension for generated chunks and local AMD modules
--assetFileNames <pattern> Name pattern for emitted assets
--banner <text> Code to insert at top of bundle (outside wrapper)
--chunkFileNames <pattern> Name pattern for emitted secondary chunks
Expand Down
17 changes: 17 additions & 0 deletions docs/999-big-list-of-options.md
Expand Up @@ -1331,6 +1331,23 @@ export default {
// -> def(['dependency'],...
```

**output.amd.keepExtension**<br> Type: `boolean`<br> CLI: `--amd.keepExtension`<br> Default: `false`

Add `.js` extension for generated chunks and local AMD modules:
lukastaegert marked this conversation as resolved.
Show resolved Hide resolved

```js
// rollup.config.js
export default {
...,
format: 'amd',
amd: {
keepExtension: true
}
};

// -> define(['./chunk-or-local-file.js', 'dependency', 'third/dependency'],...
```

#### output.esModule

Type: `boolean`<br> CLI: `--esModule`/`--no-esModule`<br> Default: `true`
Expand Down
2 changes: 1 addition & 1 deletion src/Chunk.ts
Expand Up @@ -938,7 +938,7 @@ export default class Chunk {
options: NormalizedOutputOptions,
snippets: GenerateCodeSnippets
): void {
const stripKnownJsExtensions = options.format === 'amd';
const stripKnownJsExtensions = options.format === 'amd' && !options.amd.keepExtension;
for (const [module, code] of this.renderedModuleSources) {
for (const { node, resolution } of module.dynamicImports) {
const chunk = this.chunkByModule.get(resolution as Module);
Expand Down
6 changes: 4 additions & 2 deletions src/finalisers/amd.ts
Expand Up @@ -3,7 +3,7 @@ import type { NormalizedOutputOptions } from '../rollup/types';
import getCompleteAmdId from './shared/getCompleteAmdId';
import { getExportBlock, getNamespaceMarkers } from './shared/getExportBlock';
import getInteropBlock from './shared/getInteropBlock';
import removeExtensionFromRelativeAmdId from './shared/removeExtensionFromRelativeAmdId';
import updateExtensionForRelativeAmdId from './shared/updateExtensionForRelativeAmdId';
import warnOnBuiltins from './shared/warnOnBuiltins';
import type { FinaliserOptions } from './index';

Expand Down Expand Up @@ -35,7 +35,9 @@ export default function amd(
}: NormalizedOutputOptions
): Bundle {
warnOnBuiltins(warn, dependencies);
const deps = dependencies.map(m => `'${removeExtensionFromRelativeAmdId(m.id)}'`);
const deps = dependencies.map(
m => `'${updateExtensionForRelativeAmdId(m.id, amd.keepExtension)}'`
);
const args = dependencies.map(m => m.name);
const { n, getNonArrowFunctionIntro, _ } = snippets;

Expand Down
3 changes: 3 additions & 0 deletions src/finalisers/shared/addJsExtension.ts
@@ -0,0 +1,3 @@
export default function addJsExtension(name: string): string {
return !name.endsWith('.js') ? name + '.js' : name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You added a new logic here, was this intended? E.g. if you have an external dependency dep.amd you would end up with dep.amd.js. And if the user decides to use chunk names without extensions, the extensions would be added anyway in the import, which will probably not work. What is the rationale? Also, it is not tested.

Copy link
Contributor Author

@wh1tevs wh1tevs Aug 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extension depends on output.amd.keepExtension, it described in updateExtensionForRelativeAmdId.
If keepExtension is true and module is local dependency (like ./foo), extension will be added, if it package, external or id (like bar or lib/baz/quux) extension will be removed,

for example:

import foo from './foo';
import bar from 'bar';

(output.amd.keepExtension: true)

define(['./foo.js', 'bar'], function (foo, bar) {})

(output.amd.keepExtension: false) // default

define(['./foo', 'bar'], function (foo, bar) {})

It partially tested in this commit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

partially

Ah I see, the case that is not tested (and which coverage complains about) is not what I thought but actually the combination relative id + js extension present + keep extension. Maybe you can add this for coverage.

The problem I saw is that
a) there may already be a different extension, so adding the extension .js is actually wrong, and
b) there are multiple chunks (e.g. a non-external dynamic import) and the user sets e.g. output.chunkFileNames: "[name].amd". Then "keepExtension" would probably add a wrong extension.

But then again, they should not use keepExtension in that case, so I convinced myself that you are right and this is irrelevant (also the way you wrote the docs supports this). Proposal to make this clearer: Change the name to "amd.keepJsExtension" to make clear this is about the presence or absence of ".js"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposal to make this clearer: Change the name to "amd.keepJsExtension" to make clear this is about the presence or absence of ".js"?

I think it should be amd.addJsExtension or amd.foreceJsExtensionForImports

a) there may already be a different extension, so adding the extension .js is actually wrong, and

I do not quite understand what is at stake. If you talking about dependencies in define, then only js modules will get there. Give me a example and i add tests.

b) there are multiple chunks (e.g. a non-external dynamic import) and the user sets e.g. output.chunkFileNames: "[name].amd". Then "keepExtension" would probably add a wrong extension.

No, if amd.keepExtension: false. If true, result should be [name].amd.js, i add test just in case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amd.foreceJsExtensionForImports

That is an even better name

}
8 changes: 0 additions & 8 deletions src/finalisers/shared/removeExtensionFromRelativeAmdId.ts

This file was deleted.

16 changes: 16 additions & 0 deletions src/finalisers/shared/updateExtensionForRelativeAmdId.ts
@@ -0,0 +1,16 @@
import addJsExtension from './addJsExtension';
import removeJsExtension from './removeJsExtension';

// AMD resolution will only respect the AMD baseUrl if the .js extension is omitted.
// The assumption is that this makes sense for all relative ids:
// https://requirejs.org/docs/api.html#jsfiles
export default function updateExtensionForRelativeAmdId(
id: string,
keepExtension: boolean
): string {
if (id[0] !== '.') {
return id;
}

return keepExtension ? addJsExtension(id) : removeJsExtension(id);
}
6 changes: 4 additions & 2 deletions src/finalisers/umd.ts
Expand Up @@ -5,10 +5,10 @@ import type { GenerateCodeSnippets } from '../utils/generateCodeSnippets';
import getCompleteAmdId from './shared/getCompleteAmdId';
import { getExportBlock, getNamespaceMarkers } from './shared/getExportBlock';
import getInteropBlock from './shared/getInteropBlock';
import removeExtensionFromRelativeAmdId from './shared/removeExtensionFromRelativeAmdId';
import { keypath } from './shared/sanitize';
import { assignToDeepVariable } from './shared/setupNamespace';
import trimEmptyImports from './shared/trimEmptyImports';
import updateExtensionForRelativeAmdId from './shared/updateExtensionForRelativeAmdId';
import warnOnBuiltins from './shared/warnOnBuiltins';
import type { FinaliserOptions } from './index';

Expand Down Expand Up @@ -73,7 +73,9 @@ export default function umd(

warnOnBuiltins(warn, dependencies);

const amdDeps = dependencies.map(m => `'${removeExtensionFromRelativeAmdId(m.id)}'`);
const amdDeps = dependencies.map(
m => `'${updateExtensionForRelativeAmdId(m.id, amd.keepExtension)}'`
);
const cjsDeps = dependencies.map(m => `require('${m.id}')`);

const trimmedImports = trimEmptyImports(dependencies);
Expand Down
4 changes: 4 additions & 0 deletions src/rollup/types.d.ts
Expand Up @@ -631,6 +631,8 @@ export type AmdOptions = (
}
) & {
define?: string;
} & {
keepExtension?: boolean;
};

export type NormalizedAmdOptions = (
Expand All @@ -644,6 +646,8 @@ export type NormalizedAmdOptions = (
}
) & {
define: string;
} & {
keepExtension: boolean;
};

export interface OutputOptions {
Expand Down
15 changes: 12 additions & 3 deletions src/utils/options/normalizeOutputOptions.ts
Expand Up @@ -225,10 +225,17 @@ const getPreserveModulesRoot = (
};

const getAmd = (config: OutputOptions): NormalizedOutputOptions['amd'] => {
const mergedOption: { autoId: boolean; basePath: string; define: string; id?: string } = {
const mergedOption: {
autoId: boolean;
basePath: string;
define: string;
id?: string;
keepExtension: boolean;
} = {
autoId: false,
basePath: '',
define: 'define',
keepExtension: false,
...config.amd
};

Expand Down Expand Up @@ -256,13 +263,15 @@ const getAmd = (config: OutputOptions): NormalizedOutputOptions['amd'] => {
normalized = {
autoId: true,
basePath: mergedOption.basePath,
define: mergedOption.define
define: mergedOption.define,
keepExtension: mergedOption.keepExtension
};
} else {
normalized = {
autoId: false,
define: mergedOption.define,
id: mergedOption.id
id: mergedOption.id,
keepExtension: mergedOption.keepExtension
};
}
return normalized;
Expand Down
7 changes: 7 additions & 0 deletions test/form/samples/amd-keep-extension/_config.js
@@ -0,0 +1,7 @@
module.exports = {
description: 'keep extension for AMD modules',
options: {
external: ['./foo', 'baz/quux'],
output: { interop: 'default', amd: { keepExtension: true } }
}
};
7 changes: 7 additions & 0 deletions test/form/samples/amd-keep-extension/_expected/amd.js
@@ -0,0 +1,7 @@
define(['./foo.js', 'baz/quux'], (function (foo, baz) { 'use strict';

const bar = 42;

console.log(foo, bar, baz);

}));
8 changes: 8 additions & 0 deletions test/form/samples/amd-keep-extension/_expected/cjs.js
@@ -0,0 +1,8 @@
'use strict';

var foo = require('./foo');
var baz = require('baz/quux');

const bar = 42;

console.log(foo, bar, baz);
6 changes: 6 additions & 0 deletions test/form/samples/amd-keep-extension/_expected/es.js
@@ -0,0 +1,6 @@
import foo from './foo';
import baz from 'baz/quux';

const bar = 42;

console.log(foo, bar, baz);
8 changes: 8 additions & 0 deletions test/form/samples/amd-keep-extension/_expected/iife.js
@@ -0,0 +1,8 @@
(function (foo, baz) {
'use strict';

const bar = 42;

console.log(foo, bar, baz);

})(foo, baz);
18 changes: 18 additions & 0 deletions test/form/samples/amd-keep-extension/_expected/system.js
@@ -0,0 +1,18 @@
System.register(['./foo', 'baz/quux'], (function () {
'use strict';
var foo, baz;
return {
setters: [function (module) {
foo = module["default"];
}, function (module) {
baz = module["default"];
}],
execute: (function () {

const bar = 42;

console.log(foo, bar, baz);

})
};
}));
11 changes: 11 additions & 0 deletions test/form/samples/amd-keep-extension/_expected/umd.js
@@ -0,0 +1,11 @@
(function (global, factory) {
typeof exports === 'object' && typeof module !== 'undefined' ? factory(require('./foo'), require('baz/quux')) :
typeof define === 'function' && define.amd ? define(['./foo.js', 'baz/quux'], factory) :
(global = typeof globalThis !== 'undefined' ? globalThis : global || self, factory(global.foo, global.baz));
})(this, (function (foo, baz) { 'use strict';

const bar = 42;

console.log(foo, bar, baz);

}));
1 change: 1 addition & 0 deletions test/form/samples/amd-keep-extension/bar.js
@@ -0,0 +1 @@
export const bar = 42;
5 changes: 5 additions & 0 deletions test/form/samples/amd-keep-extension/main.js
@@ -0,0 +1,5 @@
import foo from './foo';
import { bar } from './bar';
import baz from 'baz/quux';

console.log(foo, bar, baz);
3 changes: 2 additions & 1 deletion test/function/samples/output-options-hook/_config.js
Expand Up @@ -18,7 +18,8 @@ module.exports = {
assert.deepStrictEqual(JSON.parse(JSON.stringify(options)), {
amd: {
define: 'define',
autoId: false
autoId: false,
keepExtension: false
},
assetFileNames: 'assets/[name]-[hash][extname]',
chunkFileNames: '[name]-[hash].js',
Expand Down