feat: add option to keep extensions for amd #4607

wh1tevs · 2022-08-15T05:25:41Z

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

List any relevant issue numbers:
resolves #4606

Description

lukastaegert

Nice one! Not sure if you are done yet, three things:

There is another usage of removeExtensionFromRelativeAmdId in umd.ts. But as we want to observe this flag anyway wherever this function is used, maybe change the function to something like updateExtensionForAmd(id: string, keepExtension: boolean)?
And there is another place where we strip AMD extensions, this time for dynamic imports. This happens in Chunk.ts, look for stripKnownJsExtensions.
We need a test for this. Adding a test is really simple, in this case we should go for a form test. Basically add another folder in test/form/samples with content similar to the other tests. Add your option in _config.js via options.output.amd. If you put both an external static and an external dynamic import into your main.js (can be the same target), then it should be possible to cover everything with a single test.

lukastaegert · 2022-08-15T06:06:24Z

Also, there are some tests broken due to the new option, but they should be easy to fix.

codecov · 2022-08-15T10:29:34Z

Codecov Report

Merging #4607 (22a6dfb) into master (eb1fab7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4607   +/-   ##
=======================================
  Coverage   98.87%   98.87%           
=======================================
  Files         209      210    +1     
  Lines        7368     7373    +5     
  Branches     2103     2106    +3     
=======================================
+ Hits         7285     7290    +5     
  Misses         27       27           
  Partials       56       56

Impacted Files	Coverage Δ
src/Chunk.ts	`100.00% <100.00%> (ø)`
src/finalisers/amd.ts	`100.00% <100.00%> (ø)`
src/finalisers/shared/addJsExtension.ts	`100.00% <100.00%> (ø)`
...nalisers/shared/updateExtensionForRelativeAmdId.ts	`100.00% <100.00%> (ø)`
src/finalisers/umd.ts	`100.00% <100.00%> (ø)`
src/utils/options/normalizeOutputOptions.ts	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

lukastaegert

Looks really good, but one question remains, see my comment.
Beyond that, the only thing missing is documentation of the option. There are three places where it needs to be added:

In the big list of options in /docs next to the other amd options (should be sorted alphabetically),
In help.md (this is sorted alphabetically as well)
In the copy of help.md that is part of the CLI documentation in /docs

lukastaegert · 2022-08-15T12:37:04Z

src/finalisers/shared/addJsExtension.ts

@@ -0,0 +1,3 @@
+export default function addJsExtension(name: string): string {
+	return !name.endsWith('.js') ? name + '.js' : name;


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.

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

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"?

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

amd.foreceJsExtensionForImports

That is an even better name

lukastaegert

Just some small nits left, but this is nearly ready for release. Thanks for bearing with me!

lukastaegert · 2022-08-16T04:41:33Z

cli/help.md

@@ -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


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

docs/999-big-list-of-options.md

lukastaegert · 2022-08-16T04:44:50Z

src/finalisers/shared/addJsExtension.ts

@@ -0,0 +1,3 @@
+export default function addJsExtension(name: string): string {
+	return !name.endsWith('.js') ? name + '.js' : name;


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"?

lukastaegert · 2022-08-31T04:37:41Z

I changed the option name for output.amd.forceJsExtensionForImports and added the missing test case so that coverage should be fine now. Will release this if CI runs through.

feat: add option to keep extensions for amd

b543795

lukastaegert reviewed Aug 15, 2022

View reviewed changes

wh1tevs added 3 commits August 15, 2022 17:08

update extension resolving for amd

b331d2d

add tests

913312a

fix 'output-options-hook' test

4289689

lukastaegert reviewed Aug 15, 2022

View reviewed changes

add docs

f3a1d0a

lukastaegert reviewed Aug 16, 2022

View reviewed changes

lukastaegert and others added 2 commits August 30, 2022 14:31

Update docs/999-big-list-of-options.md

d27fd4e

Change extension name to forceJsExtensionForImports

a412009

lukastaegert force-pushed the feat/keep-extension-for-amd branch from 3ede154 to a412009 Compare August 30, 2022 12:46

Improve coverage

4427349

Merge branch 'master' into feat/keep-extension-for-amd

22a6dfb

lukastaegert approved these changes Aug 31, 2022

View reviewed changes

lukastaegert enabled auto-merge (squash) August 31, 2022 04:44

lukastaegert merged commit 71eefd1 into rollup:master Aug 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add option to keep extensions for amd #4607

feat: add option to keep extensions for amd #4607

wh1tevs commented Aug 15, 2022

lukastaegert left a comment

lukastaegert commented Aug 15, 2022

codecov bot commented Aug 15, 2022 •

edited

lukastaegert left a comment

lukastaegert Aug 15, 2022

wh1tevs Aug 15, 2022 •

edited

lukastaegert Aug 16, 2022

wh1tevs Aug 16, 2022

lukastaegert Aug 16, 2022

lukastaegert left a comment

lukastaegert Aug 16, 2022

lukastaegert Aug 16, 2022

lukastaegert commented Aug 31, 2022

		@@ -0,0 +1,3 @@
		export default function addJsExtension(name: string): string {
		return !name.endsWith('.js') ? name + '.js' : name;

feat: add option to keep extensions for amd #4607

feat: add option to keep extensions for amd #4607

Conversation

wh1tevs commented Aug 15, 2022

Description

lukastaegert left a comment

Choose a reason for hiding this comment

lukastaegert commented Aug 15, 2022

codecov bot commented Aug 15, 2022 • edited

Codecov Report

lukastaegert left a comment

Choose a reason for hiding this comment

lukastaegert Aug 15, 2022

Choose a reason for hiding this comment

wh1tevs Aug 15, 2022 • edited

Choose a reason for hiding this comment

lukastaegert Aug 16, 2022

Choose a reason for hiding this comment

wh1tevs Aug 16, 2022

Choose a reason for hiding this comment

lukastaegert Aug 16, 2022

Choose a reason for hiding this comment

lukastaegert left a comment

Choose a reason for hiding this comment

lukastaegert Aug 16, 2022

Choose a reason for hiding this comment

lukastaegert Aug 16, 2022

Choose a reason for hiding this comment

lukastaegert commented Aug 31, 2022

codecov bot commented Aug 15, 2022 •

edited

wh1tevs Aug 15, 2022 •

edited