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

Allow setting of copy instructions per bundle #45

Merged
merged 4 commits into from
Nov 26, 2024

Conversation

max-moser
Copy link
Contributor

@max-moser max-moser commented Sep 24, 2024

Start implementing the configurable copy instructions for webpack projects.

Configuring this theme in webpack.py:

theme = WebpackThemeBundle(
    import_name=__name__,
    folder="theme/assets",
    default="semantic-ui",
    themes={
        "semantic-ui": {
            "entry": {
                # ...
            },
            "dependencies": {
                # ...
            },
            "aliases": {
                # ...
            },
            "copy": [
                {"from": "../node_modules/tinymce/skins/content/default/content.css", "to": "js/skins/content/default"},
            ]
        },
    },
)

Results in the copy instructions to be generated in the config.json file:

{
  "aliases": {
    // ...
  },
  "build": {
    // ...
  },
  "copy": [
    {
      "from": "../node_modules/tinymce/skins/content/default/content.css",
      "to": "js/skins/content/default"
    }
  ],
  "entry": {
    // ...
  }
}

Limitations

Right now, there is no validation about the to/from locations not going out of bounds, and the values are simple string literals.
Also, it will of course require another PR for Invenio-Assets so that the new config is actually used in webpack.config.js.

Edit: Sanity checks have been implemented in webpack.config.js: inveniosoftware/invenio-assets#173

Checking in Python?

The flask_webpackext.WebpackBundleProject automatically sets some paths in the project's configuration based on the Flask application which may be useful for out-of-bounds checks.
I'm not entirely sure if that is sound from a hierarchy perspective though – it'd be rough for pywebpack to rely on values from flask-webpackext.

Edit: I just implemented the out-of-bounds checks in JS, see above.

Checking in JS?

Maybe the validation should happen in webpack.config.js?
As long as the check (and possible abort) happens before any files are actually copied, I think it should be fine.
Sure that can be swapped out, but changing the WebpackBundleProject in Python is just as easy, it just requires changing the WEBPACKEXT_PROJECT config value.

Ease of use?

Edit: See next comment

Another question is how to make the configuration more usable.
As described above, the to/from values are just plain string literals which requires some knowledge about the structure in the instance_path to be useful at all.
In the current webpack.config.js, the from paths are usually based on the current directory, and the to paths are usually based on the config.build.assetsPath:

        {
          from: path.resolve(__dirname, '../node_modules/tinymce/skins/content/default/content.css'),
          to: path.resolve(config.build.assetsPath, 'js/skins/content/default'),
        },

Perhaps it would be nice to enable the use of certain "dynamic" base paths.

@max-moser
Copy link
Contributor Author

Update: Keeping in mind that the base path for the copy instructions is the directory for config.json/webpack.config.js, the usability is actually not terrible.
I don't actually expect this feature to be used to an extent where adding shortcuts would save any meaningful amount of time for developers.

Maybe this could be relevant, if the assetsPath is "completely unrelated" to the location of the build config?
To avoid over-engineering something nobody is going to need, I'd shelve this thought until some actual use cases come along.

* because copy instructions may vary based on the exact JS code in
  question (e.g. TinyMCE and PDF.js), they are placed in bundles
* because it's easier to control the behaviour of Python, e.g. in
  testing scenarios
@ntarocco ntarocco merged commit 4ebd69a into inveniosoftware:master Nov 26, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants