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
implement amd.autoId
/amd.basePath
options
#3867
Conversation
src/Chunk.ts
Outdated
@@ -719,6 +719,7 @@ export default class Chunk { | |||
dependencies: [...this.renderedDependencies!.values()], | |||
exports: this.renderedExports!, | |||
hasExports, | |||
id: this.id as string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I correct assuming id
is always a string at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, yes.
Codecov Report
@@ Coverage Diff @@
## master #3867 +/- ##
=======================================
Coverage 97.07% 97.07%
=======================================
Files 185 187 +2
Lines 6523 6539 +16
Branches 1889 1901 +12
=======================================
+ Hits 6332 6348 +16
Misses 101 101
Partials 90 90
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will need to test this thoroughly. So this looks like a very good start, I am mostly wondering about the interface and interaction with the amd.id option.
In a way, it feels weird to have this special option where we could integrate it into the amd.id option, e.g. via a placeholder pattern similar to what we have for the various filenames. Just prepending the id seems—arbitrary to me.
Another point we might add to this PR is to finally add a warning if a build produces several chunks with the same id, telling the user how to fix it.
Another thing I am wondering about: Isn't the whole point of using ids to be able to import files differently? I.e. shouldn't files be imported by their id
instead of their file names if I am using ids? Because if you are using the ids, then you can concatenate the files and it will still work while if you are using file names, it will not be able to import the files.
We can actually write tests for this using the runAmd
option you can use in chunking-form tests. This will actually run the code-split built against requirejs.
So we could write two tests:
- If I am using file names as ids, does everything still work? (there should not be an issue at the moment, I guess)
- If I am using arbitrary ids but concatenate the files in a
generateBundle
hook (i.e. delete everything from the bundle and emit a new file that concatenates everything asmain.js
, because that is what runAmd is looking for), does it still work?
src/Chunk.ts
Outdated
@@ -719,6 +719,7 @@ export default class Chunk { | |||
dependencies: [...this.renderedDependencies!.values()], | |||
exports: this.renderedExports!, | |||
hasExports, | |||
id: this.id as string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, yes.
src/finalisers/amd.ts
Outdated
@@ -5,14 +5,15 @@ import { getExportBlock, getNamespaceMarkers } from './shared/getExportBlock'; | |||
import getInteropBlock from './shared/getInteropBlock'; | |||
import warnOnBuiltins from './shared/warnOnBuiltins'; | |||
|
|||
function removeExtension(name: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call this removeJsExtension
, the name confused be as you definitely want to keep non-.js
extensions!
I was wondering how we could do it in a non breaking way.
Sounds good, or maybe a callback if interpreting characters are a placeholder would be breaking.
👍
I think for dynamic requires the default behaviour is to build the url to the file from the ID. We concat all files together that are in our entry point (one of which is the entry amd file from rollup), so they need unique ids for that.
👍 |
Pushed a commit to switch to a pattern. Just realised the umd finaliser will also need updating. Also if feels a bit strange that |
7127af0
to
e3cf09c
Compare
} | ||
}, | ||
additionalFormats: ['umd'], | ||
runAmd: exports => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not call the function. The correct syntax is
{
runAmd: {
exports(exports) {
// do something with exports
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will find that exports
is undefined for you. The reason is that indeed even though it loads the main
chunk, it does not load the main module because the id does not match. If you add logs to your code, you will see that no code is executed.
To fix this, we should probably extend the runAmd object so that you can specify also the id of the main entry if it is not main
, but it will require some more digging how to handle this in requirejs.
I spend some more time digging into requirejs, so here is a revised version of the test loader that allows you to specify an alternative entry id via
The new test runner will still try to load the async function generateAndTestBundle(bundle, outputOptions, expectedDir, config) {
await bundle.write(outputOptions);
if (outputOptions.format === 'amd' && config.runAmd) {
const entryAmdId = config.runAmd.id || entryFile;
const requirejs = require('requirejs');
requirejs.config({ baseUrl: outputOptions.dir });
global.assert = require('assert');
requirejs(entryFile);
const exports = requirejs(entryAmdId);
if (typeof exports === 'undefined') {
throw new Error(
`Entry AMD id "${entryAmdId}" was not defined by entry file "${entryFile}", registered ids: ${Object.keys(
global.requirejsVars.requirejs.s.contexts._.registry
)
.map(id => `"${id}"`)
.join(', ')}.`
);
}
if (config.runAmd.exports) {
await config.runAmd.exports(exports);
}
delete require.cache[require.resolve('requirejs')];
delete global.requirejsVars;
delete global.assert;
}
assertDirectoriesAreEqual(outputOptions.dir, expectedDir);
} Playing around more, this confirmed my suspicion: If you keep using the chunk file names as ids when importing files (as your current implementation does), the actual code will just not be executed. It still works if you use But I could confirm that everything works as expected (at least if you manually concatenate for ids other than So assume you have chunks
and we assume that you use AMD ids So what needs to be done to make this PR functional is to use the chunks AMD.id instead of the file name, and this needs to happen outside the finalizers as part of the regular Chunk rendering process. Depending on how this is done, one could consider moving the remaining AMD id logic out of the finalizers as well and just make the AMD id another parameter to pass in. |
test/chunking-form/index.js
Outdated
@@ -10,7 +10,7 @@ runTestSuiteWithSamples('chunking form', path.resolve(__dirname, 'samples'), (di | |||
() => { | |||
let bundle; | |||
|
|||
for (const format of FORMATS) { | |||
for (const format of [...FORMATS, ...(config.additionalFormats || [])]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you use this to allow UMD
here but really I do not think it makes a lot of sense considering UMD just does not support code-splitting. Instead, use a regular form test for the non-code-splitting case.
@@ -0,0 +1,17 @@ | |||
module.exports = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your test names are switched: The code-split test is not doing the code splitting while the other one does
So just to confirm I'm understanding. With the example
if the complete ids were
Yeh this makes me think making it templateable isn't super useful? Initially I supported a prefix because the output from rollup may be placed in a nested directory (which happens in our current setup), so the above output from rollup would end up as something like
meaning now, the complete ids would need to be
Not following this. Using the |
What I want to say is:
And I think the latter IS useful. But to make it work, one has to switch to using AMD ids instead of file names for all imports as well as adding the AMD ids to the rendered output. So far, you only did the latter. If you think this is too much for this PR, maybe how about a compromise like allowing to use |
By the way, this is what a concatenation plugin could look like which you could use in a test: {
name: 'concatenate',
generateBundle(options, bundle) {
if (options.format === 'amd') {
const concatenatedCode = Object.keys(bundle)
.map(chunkName => bundle[chunkName].code)
.join('\n');
for (const chunkName of Object.keys(bundle)) {
delete bundle[chunkName];
}
this.emitFile({ type: 'asset', fileName: 'main.js', source: concatenatedCode });
}
}
} It actually produced runnable code in your implementation if you set |
Ah I think I understand now.
Is this the case though? It looks like on the repl (and what I'm seeing in our app) that the imports are all relative ( I think (but maybe misunderstood) you're suggesting if we were prepending new Promise(function (resolve, reject) { require(['./maths-0ec2b017'], resolve, reject) }) to new Promise(function (resolve, reject) { require(['rollup-bundle/maths-0ec2b017'], resolve, reject) }) ?
In our use case we need to support the pattern/prefix. Also if requirejs requires the part after the final |
Exactly
Well, it is like this:
So these are two very distinct use cases. On top of that, the first use-case would still work with relative imports while the second would require full id imports. So maybe in the end, maybe we also need to define if we use relative imports or absolute imports. But going with the absolute minimum that would keep the future open, maybe
with the option to extend this to
|
This is exactly our use case at the moment
Yep, but I'm wondering why someone would want arbitrary strings?
ah right. Hadn't noticed this case. It doesn't exist for us right now because the dynamic chunks aren't concat'd into the entry file. amd: {
id: true // use automatic ids,
idPrefix: 'some/string' // just some string that is prefixed to the id
} So with this config ( |
This is very nice work! It looks like it might solve the need I had in #3333. I tested it out with our codebase, using One consideration I wanted to report, to see what you think about it, relates to AMD module IDs and dependencies. We have a situation where we have files that build 1 JS file per entrypoint (there can be a variable amount of entrypoints), and 2 shared JS files called If I build with this PR with config: const outputOptions = {
format: 'amd',
amd: {
autoId: true,
basePath: 'foo'
}
} ...then an entrypoint bundle called // vendor.js
define('foo/vendor', [], function () {})
// common.js
define('foo/common', ['./vendor'], function (vendor) {})
// bar.js
define('foo/bar', ['./vendor', './common'], function (vendor, common) {}) In this case, the AMD ID isn't being used to import dependencies like Is there a use case for using the AMD module IDs, not the relative paths, for importing dependencies? // vendor.js
define('foo/vendor', [], function () {})
// common.js
define('foo/common', ['foo/vendor'], function (vendor) {})
// bar.js
define('foo/bar', ['foo/vendor', 'foo/common'], function (vendor, common) {}) |
As I already noted above, I did some experiments, the results were:
Not sure if this answers your question. |
One of the examples does
Yeh like Lukas mentioned earlier, but this adds a lot more complexity and usually takes up more space, so we were thinking that could be an additional option |
Thanks for the replies, I'll give it some more testing. As I recall, one of the issues I hit was the usage of a single AMD namespace for everything, so module names like This problem was solved for production builds by including the fingerprinted module filename in its AMD ID, which fits a process where 1 file = 1 AMD module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good and I agree with your suggestions. Beyond what I wrote in the comments, here are some other final points to address:
- It would be really nice to have an error when the amd.id is used in a multi-file build. The best place to throw such an error IMO is in the
validateOptionsForMultiChunkOutput
function in Bundle.ts. - Coverage could still be improved in some places, see the codecov report:
- extracting how pattern functions are handled means that they now need to be tested for all three cases if they are correctly applied. It appears to be untested for entryFileNames. Maybe some existing test could just be extended to accommodate this.
- the error cases in normalizeOutputOptions are not tested. I know this is kind of a chore, they are usually done using simple
function
tests.
Thanks a lot for your hard work, this is a really great feature!
@@ -712,13 +710,18 @@ export default class Chunk { | |||
}); | |||
} | |||
|
|||
if (!this.id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big fan of internal errors that are impossible to test, but there are other opinions out there as well. The price of course are non-null assertions. In a future version of Rollup, maybe we can move away from classes towards data types that evolve as the bundle evolves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh I had that initially but noticed a few more internal errors. Happy to remove if you want
maybe we can move away from classes towards data types that evolve as the bundle evolves.
sounds good
Done, but I'm wondering if this would be breaking? Right now we set Think I've addressed everything now except the coverage. Will do that now |
amd.idFromChunkName
optionamd.autoId
/amd.basePath
options
I think the only reason coverage is down in chunk.ts now is because of the internal error. We could add a |
added the comment because saw the same is done for other internal errors. |
You are very right! I am sorry I did not even consider this could happen. Then we should make the error a warning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, just the warning/error, then we are good to go from my side.
{ | ||
basePath: '' | ||
} | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This PR contains:
Are tests included?
Breaking Changes?
Related issues: #3333
Description
It's currently possible to set an
amd.id
, but when there are multiple chunks they all get the same id, which isn't helpful.This PR implements
idFromChunkName
, which whentrue
causesamd.id
to become a prefix, and the chunk name is then used in the id, so that the chunk filename matches the id in the AMD id. This makes it possible to use AMD ids for builds that introduce code splitting.To achieve this the chunk id is now passed to the finalizers.