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

Include built Assets in Releases? #262

Open
tomjn opened this issue Mar 11, 2024 · 5 comments · May be fixed by #267
Open

Include built Assets in Releases? #262

tomjn opened this issue Mar 11, 2024 · 5 comments · May be fixed by #267

Comments

@tomjn
Copy link
Contributor

tomjn commented Mar 11, 2024

Currently we build assets and commit them to the repo, but this has some unintended consequences:

  • releases aren't as simple as tags as they now require us to build or automate building assets
  • a project I'm on uses Webpack 4 and doesn't support chaining operators ?. but the built assets for this package started using ?. as of v0.5 making it difficult to upgrade

Do we want to continue building committing and releasing the assets?

@tomjn
Copy link
Contributor Author

tomjn commented Mar 11, 2024

@mattheu @Sephsekla curious what your thoughts on this are

@Sephsekla
Copy link
Contributor

@tomjn I agree here, the whole use case is that we'll want to be importing the JS components, so this is going to be compiled by webpack or similar anyway.

We'll just need to bump it to 0.7 since this is a breaking change.

@mattheu
Copy link
Member

mattheu commented Mar 13, 2024

Hmm i'm not sure about this. Are you installing from NPM or from Github?

It's true that a release is not a simple tag but the build step is run automatically as part of the release process.. And the dist version pushed to NPM. Although I note NOT included in the tagged version in Github.

I think the original idea back when this project was started was indeed to simply import the source components into your project, but I recall that we had lots of problems simply including the source files into another project. Aside from compatibility, I also had issues configuring webpack to actually get this to work at all.

So when we switched to recommending installation from NPM, we introduced an extra build step as part of the release... which should solve much of the compatibility/configuration problem.e.g. I would expect that optional chaining operators are not an issue if you're using the built version. If you're using the version from NPM and are facing these issues, that's probably something we should look at fixing.

Having said that - I'm open to suggestions here as to what the best practice for this type of library is. I'm just concerned that this is reverting back to how we used to do things that was problematic.

We'll just need to bump it to 0.7 since this is a breaking change.

Is that because a minor version change below 1.0.0 can include breaking changes... this small difference always catches me out :)

@tomjn
Copy link
Contributor Author

tomjn commented Mar 13, 2024

I would expect that optional chaining operators are not an issue if you're using the built version.

It's the built asset that introduces them

@mattheu
Copy link
Member

mattheu commented Mar 13, 2024

I guess this project is not configured to avoid using them. (We're using the WordPress preset env config and I'm not sure exactly how it's configured). Either way - this type of compatibility issue is always something that could happen and probably should handle at your end - can you add the babel plugin to add support for this to your project.. I don't think changing the way we build assets for releases solves the problem at hand.

That said - I'm open to improving things. If we can get rid of the build step then it will be easier to maintain for sure.

If I recall what the problem was - I think I was trying to use npm link to test changes in the project locally, and webpack didn't really work great across symlinks and using a build step solved that.

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 a pull request may close this issue.

3 participants