add file loader #135
add file loader #135
Conversation
internal/bundler/bundler.go
Outdated
| PrettyPath: prettyPath, | ||
| Contents: contents, | ||
| } | ||
|
|
||
| // Get the file extension | ||
| extension := "" | ||
| if lastDot := strings.LastIndexByte(path, '.'); lastDot >= 0 { | ||
| extension = path[lastDot:] | ||
| if lastDot := strings.LastIndexByte(sourcePath, '.'); lastDot >= 0 { |
There was a problem hiding this comment.
this line is probably could be simplified with https://golang.org/pkg/path/#Ext thinking @evanw any reason we don't just use it?
There was a problem hiding this comment.
Sounds good to me. I probably just wasn't aware of it existing.
internal/bundler/bundler_test.go
Outdated
| t.Fatalf("%s != %s", a, b) | ||
| stringA := fmt.Sprintf("%v", a) | ||
| stringB := fmt.Sprintf("%v", b) | ||
| t.Fatalf(diff.Diff(stringA, stringB)) |
There was a problem hiding this comment.
i've added this change because it's hard to diff any whitespace differences in tests when it happens
cmd/esbuild/main.go
Outdated
| @@ -46,7 +46,7 @@ Options: | |||
| --jsx-factory=... What to use instead of React.createElement | |||
| --jsx-fragment=... What to use instead of React.Fragment | |||
| --loader:X=L Use loader L to load file extension X, where L is | |||
| one of: js, jsx, ts, tsx, json, text, base64, dataurl | |||
| one of: js, jsx, ts, tsx, json, text, base64, url, dataurl | |||
There was a problem hiding this comment.
This feature seems similar to the Webpack file-loader plugin, so I think people will understand the name file better than url. There is also a Rollup url plugin which does something different (it does what dataurl does in esbuild), so that may also be confusing for people coming from Rollup.
There was a problem hiding this comment.
that's true! url-loader seems to have more specific feature in webpack world https://webpack.js.org/loaders/url-loader/ we could use url to automatically switch between dataurl and file in the future i guess. i'll rename this to file
internal/bundler/bundler.go
Outdated
| PrettyPath: prettyPath, | ||
| Contents: contents, | ||
| } | ||
|
|
||
| // Get the file extension | ||
| extension := "" | ||
| if lastDot := strings.LastIndexByte(path, '.'); lastDot >= 0 { | ||
| extension = path[lastDot:] | ||
| if lastDot := strings.LastIndexByte(sourcePath, '.'); lastDot >= 0 { |
There was a problem hiding this comment.
Sounds good to me. I probably just wasn't aware of it existing.
internal/bundler/bundler.go
Outdated
| if targetFolder == "" { | ||
| targetFolder = path.Dir(bundleOptions.AbsOutputFile) | ||
| } | ||
| defer ioutil.WriteFile(path.Join(targetFolder, url), []byte(source.Contents), 0644) |
There was a problem hiding this comment.
The bundler isn't supposed to write files itself. The bundler returns all of the files to be written to the caller. This makes the bundler easier to unit test and also makes it easy for the API to return results without modifying the file system (see for example #139).
Doing things this way also defeats future tree shaking optimizations, which may end up removing this module entirely. In that case the file should not be emitted in the output directory since it's unreferenced.
Instead of writing to the file system, the contents of the file should be stashed somewhere associated with the file, probably just on the parseResult or ast. Then the bundler should return the file to the caller, probably as an extra BundleResult entry. But that requires generalizing BundleResult to handle non-JavaScript result files.
There was a problem hiding this comment.
thank you! yeah this is a part where i'm not really sure. i'll try generalizing BundleResult to handle non-JavaScript result files
| @@ -635,6 +637,17 @@ func run(fs fs.FS, args argsObject) { | |||
| } | |||
| args.logInfo(fmt.Sprintf("Wrote to %s (%s)", path, toSize(len(item.JsContents)))) | |||
|
|
|||
| // Write out the additional files | |||
| for _, file := range item.AdditionalFiles { | |||
There was a problem hiding this comment.
The contents of AdditionalFiles end up being written out twice because they are both written out here and duplicated to become normal files in this code elsewhere:
for _, bundle := range group {
for _, additionalFile := range bundle.AdditionalFiles {
if additionalFile.Path != "" {
results = append(results, BundleResult{additionalFile.Path, []byte(additionalFile.Contents), "", []byte(""), []AdditionalFile{}})
}
}
}That's ok, I can fix it after landing this.
| stringA := fmt.Sprintf("%v", a) | ||
| stringB := fmt.Sprintf("%v", b) | ||
| if strings.Contains(stringA, "\n") { | ||
| t.Fatal(diff.Diff(stringA, stringB)) |
There was a problem hiding this comment.
I understand why you did this, but this is going to make it harder to update tests since I've been copying and pasting the output of the tests to accept the new output, and this interleaves the old and new output. Luckily it's easy to disable by just commenting out a few lines, so I don't think it's a big problem.
Really I should figure out some system of doing snapshot-style testing so that test output can be updated automatically...
No description provided.