WIP: Refactor loader dependencies #119
Conversation
Isolates the file loader so it does no longer depend on the parser. This frees the loader to only care aout loading files, which makes it easier to inject custom loaders via the provided interface. Since the loader is now only a single function that is extracted immediately, it's been reduced to a function instead of a class.
The previous used version of babel had a bug with transpiling async methods on classes. See: babel/babel#6806 Update of babel dependencies fixes the problem
|
Hey @RianFuro, thanks for the PR! I think I'll be able to take a look at it at the end of the week |
Since the parser shouldn't have to deal with file system operations, the loader plugin now has to provide a unique id for an import, which can just be an absolute path for the default implementation.
| @@ -1,112 +1,32 @@ | |||
| // Copied from https://github.com/css-modules/css-modules-loader-core | |||
There was a problem hiding this comment.
I think we can move the loader and parser files to the src directory directly. The directory was just a copy-and-paste from the library as a hot fix and unnecessary now.
| import fs from "fs"; | ||
| import path from "path"; | ||
|
|
||
| import Parser from "./parser"; | ||
|
|
||
| class Core { |
There was a problem hiding this comment.
I love to see the Core is gone
| (err) => console.log(err) | ||
| ); | ||
| async fetchImport(importNode, relativeTo, depNr) { | ||
| const file = importNode.selector.match(importRegexp)[1].replace(/^["']|["']$/g, ""), |
There was a problem hiding this comment.
Let's split the declarations into separate statements everywhere for the readability sake
| class Core { | ||
| constructor(plugins) { | ||
| this.plugins = plugins || Core.defaultPlugins; | ||
| export function resolveRelativeImport(importee, importer, projectRoot) { |
There was a problem hiding this comment.
It seems to be an internal function. Let's not export it then
| @@ -31,11 +32,11 @@ | |||
| "postcss": "^8.0.0" | |||
| }, | |||
| "devDependencies": { | |||
| "@babel/cli": "7.12.10", | |||
| }), | ||
| tokens = this.tokensByFile[fileRelativePath]; | ||
|
|
||
| try { |
There was a problem hiding this comment.
This seems to be cleaner now 👍
| } | ||
|
|
||
| export default { | ||
| resolveId: (importee, importer, projectRoot) => { |
There was a problem hiding this comment.
Nice idea to use resolveId, it may be really useful in the future
I was about to wrap this up, but to be honest with the last 2 changes I got a little uncomfortable. I'm sure I made a logical mistake at least once during the work on my last commit, but the tests passed regardless. So I think I will take some extra time to extend the test suite against the original implementation, then move those over here so I can hand the pr off with some confidence :) |
Yeah, this is what I wanted to ask you to do |
|
Could you also update the docs according to the changes? |
Sure thing. |
| let relativeDir = path.dirname(relativeTo), | ||
| rootRelativePath = path.resolve(relativeDir, newPath), | ||
| fileRelativePath = path.resolve( | ||
| path.join(this.root, relativeDir), | ||
| newPath | ||
| ); |
There was a problem hiding this comment.
@madyankin
Btw, do you have a bit of insight on the old code? I'm not quite sure about the distinction between rootRelativePath and fileRelativePath here. They are equal as long as this.root is unset and I don't see why they need to be be different in the alternative case... so while refactoring I got rid of the distinction.
But since I don't completely trust the test suite anymore, I'm not sure if that's a correct step to take. Any idea?
There was a problem hiding this comment.
I believe I had the insight a couple of years ago or so, but I'm not quite sure now.
AFAIR, this is useful when you have CSS modules placed in some other location and want to resolve them properly, e.g., in Rails or Django frameworks.
These files were in the css-modules-loader-core package without tests. Given I copied them here as a hotfix, I hadn't enough time to cover them with.
Closes #118
Very WIP, I still need to clean up the resulting mess.
However, the loader roughly looks like I wanted it to, so I wanted to get some early feedback.
I basically moved all the logic that wasn't about loading a file to the parser class, which eliminated the weird
Corein-between altogether. As a result, the parser is now a mess, so that needs to be cleaned up still.Also, the parser still deals with absolute file paths, since I just pulled out the sources hash from the loader. That needs to be changed too, since a file might not necessarily come from disk as far as the parser is concerned.
TODOs: