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

feat - Add support for webpack 5 #33

Merged

Conversation

bebraw
Copy link
Contributor

@bebraw bebraw commented May 10, 2021

As requested by @shilman, I've added webpack 5 support to the plugin. Here are the highlights of the work:

  • I added an initial test setup for the plugin itself as that didn't exist yet though more tests should definitely be added to verify behavior
  • There's a small example to run (I developed mainly against this as logging works there but not in tests) and check
  • The current implementation supports only webpack 5 but with some extra effort webpack 4 support could be maintained. I would suggest making a new major version that has webpack 5 support only to keep things simple
  • There are some dependency updates but I've tried to keep them to minimum as the PR is quite big even without this type of work
  • For anything that's unclear, I've added a TODO. I struggled especially with webpack's typings as they aren't well exposed (mainly docstrings + the exported types are missing some I needed here)

I haven't tested the work on anything bigger so I would expect there may still be some issues to uncover. It's also unclear if webpack 5 caching will simply work or if some small change is needed. It's not clear how to test that effectively yet.

TODO:

  • Address open comments (seal, performance)
  • Add webpack 4 support (backport old logic)
  • Test webpack 4 support
  • Restore webpack 4 caching (should this be a flag?)
  • Optional - Add a performance test

Closes #30.

@bebraw
Copy link
Contributor Author

bebraw commented May 14, 2021

@shilman Ok. Likely webpack 4/5 can be done but it's not going to be very pretty. I'm going to need some nice way to handle conditional imports in TS if we go this way. I'll await more info before hacking around. 😺

@shilman
Copy link
Contributor

shilman commented May 14, 2021

Yeah if we can solve it cleanly on the storybook end, I can imagine a major breaking change will be more clean

@bebraw
Copy link
Contributor Author

bebraw commented May 14, 2021

I gave it some extra thought. A potential way to solve the webpack 4/5 API issue (it has mainly to do with internal APIs and our reliance on them) would be to implement a small adapter in between that's normalizing the few imports I need since it's easy to do handle dynamic imports in CommonJS. That's one way forward though likely typing will be a bit weird.

@7rulnik
Copy link

7rulnik commented May 14, 2021

For cache detection you can just check compiler.options.cache in apply method.

Also note, that you can use custom dependency in webpack 4 too. So the way how you modify modules could be exactly the same.

Also it's not that hard to make tests against both 4 and 5 versions. See https://github.com/7rulnik/isomorphic-env-webpack-plugin/blob/main/e2e/__tests__/plugin.test.js#L14

@shilman
Copy link
Contributor

shilman commented May 14, 2021

If we want to handle it on the storybook side it looks like there is a solution (thanks @merceyz for your expert guidance!):

{
    "dependencies": {
        "react-docgen-typescript-plugin-v5": "npm:react-docgen-typescript-plugin@^0.5.0",
        "react-docgen-typescript-plugin-v6": "npm:react-docgen-typescript-plugin@^0.6.0"
    }
}

This aliasing is supported in npm >= 6.9.0 or yarn.

@bebraw
Copy link
Contributor Author

bebraw commented May 18, 2021

I've restored support for webpack 4 including caching. Now in case it's running with something else than webpack 5, it's using the old code path. There's also a small testing setup in place to assert the plugin works against both webpack 4 and 5.

The remaining question is what to do with webpack 5 and caching. Now it's relying on the cache provided with webpack 5 (not fully tested). I could add webpack 4 style caching on top of that if you think it would be beneficial.

More notes below.


For cache detection you can just check compiler.options.cache in apply method.

Thanks. That's a good tip.

Also note, that you can use custom dependency in webpack 4 too. So the way how you modify modules could be exactly the same.

Ok, that's a good point. I've restored the old code path as it became tricky with typings and imports. webpack 5 changed the location of a couple of modules we depend on and typings changed completely as well.

Also it's not that hard to make tests against both 4 and 5 versions. See https://github.com/7rulnik/isomorphic-env-webpack-plugin/blob/main/e2e/__tests__/plugin.test.js#L14

I added a --no-save (hacked to work with Yarn) based setup that's running against a virtual file system to assert against. That said, your webpack-4 custom resolve technique looks like a good option as well.

@shilman
Copy link
Contributor

shilman commented May 18, 2021

@hipstersmoothie can we get another canary pretty please? 🙏

bebraw added 3 commits May 20, 2021 10:29
It looks like not all modules have it so it could crash here.
It looks like the other type can fail after compiled to JS.
For a reason that's not completely clear webpack emits a path here.
Better remove it.
@shilman
Copy link
Contributor

shilman commented May 20, 2021

@hipstersmoothie I've released my own canaries at @storybook/react-docgen-typescript-plugin and after a few twists & turns it appears to be working well for both webpack4/5.

@hipstersmoothie
Copy link
Owner

Here the latest [email protected]

@iamchanii
Copy link

I'm sorry to interrupt you. I just tried the latest canary build on my job project and It works fine. now I can get docs for components now.

(Storybook: 6.3.0-alpha.25, w/ builder-webpack5)

image

(Please understand that it is not English.)

@hipstersmoothie
Copy link
Owner

@bebraw @shilman This ready to go?

@shilman
Copy link
Contributor

shilman commented May 22, 2021

@hipstersmoothie Yes, I've tested it with both webpack 4 and 5 and it seems to be working great!

@hipstersmoothie
Copy link
Owner

@bebraw When you're ready I can hit merge!

@bebraw
Copy link
Contributor Author

bebraw commented May 26, 2021

@hipstersmoothie I don't have anything else in mind for this one. Since the change is backwards compatible, it can be a minor release even.

@shilman
Copy link
Contributor

shilman commented Jun 5, 2021

@hipstersmoothie what's holding this up?

@hipstersmoothie hipstersmoothie merged commit 0a95a0f into hipstersmoothie:master Jun 5, 2021
@github-actions
Copy link

github-actions bot commented Jun 5, 2021

🚀 PR was released in v1.0.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Increment the major version when merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support webpack 5
6 participants