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

chore(templates/react): add webpack entries to all templates #962

Merged
merged 4 commits into from
Oct 1, 2020

Conversation

vanbasten17
Copy link
Contributor

@vanbasten17 vanbasten17 commented Sep 29, 2020

Set as Draft PR if it's not ready to be merged.

PR best practices Reference

Description

As mentioned in the tech daily, applying the trick we did for other providers projects to every template, by adding the corresponding webchat-entry.js in @botonic/react src. (Ignore commit: 36ea33c)

  • Important: Reverted pusher to version 5.1.1 in @botonic/core as newer versions > 5.1.1 introduce breaking changes that make webchat to not work.
  • Added webpack-entries with every entry file. Then the alias like BotonicProject won't be used anymore, so the pipeline is less "magical" (the developer will be conscious that these files are to configure webpack)
  • Added name and version to every template as reported: Adding name and version fields to generated package.json #928

Context

The expected generated webchat.botonic.js after a bot build was larger than expected, causing performance issues (extra download cost when loading the bot code).

To document / Usage example

We have to take into account that developers upgrading from older versions than 0.14.0 must add the webpack-entries folder to their root project and do the proper modifications in their webpack.config.js.

Next steps

Try webpack v5.

@@ -0,0 +1,19 @@
import { DevApp } from '@botonic/react'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave all templates as they are except the ones about webchat & webviews for several reasons:

  1. minimize changes
  2. to make examples simpler, to avoid scaring developers if templates are too complex
  3. in case we refactor botonic in the future, it will be easier to modify 1 file per bot rather than 4 files per bot

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were talking about this with @marcrabat yesterday, and we finally decided to go this way because:

  1. It is a sure and simple way to enforce tree shaking of each artifact.
  2. We avoid the "magic" of webpack aliases, I think that's more confusing for the developer than having some extra files with a clear purpose. It provides clarity about how things are built and also provides flexibility in case they want to modify the entry points.
  3. This is actually more future proof than the old version, as we "eject" the entry files to the project code, we no longer need to keep that file in botonic source code, so we don't need to worry about backwards compatibility (that responsibility is no longer in botonic). Developers will be able to upgrade botonic versions in their old project without worrying about having to change their webpack config.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with separating the entry files, but I was suggesting that for the templates which have no webchat nor webviews, it was a bit confusing having so many files.

Copy link
Contributor Author

@vanbasten17 vanbasten17 Oct 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it's better to have all of them already set up in case the developer wants to add webviews or webchat later (which can be very common). Otherwise the developer should need to research these files somewhere and I think won't be favourable for his DX. I understand it can be more cumbersome to maintain, but all the files in webpack-entries should be the same for all templates and the entire folder is easy to replicate.

@vanbasten17 vanbasten17 force-pushed the fix/add-webchat-entry branch from 5b60bf2 to 0f7618c Compare October 1, 2020 08:16
@vanbasten17 vanbasten17 changed the title chore(templates/react): add webchat-entry to react and all templates chore(templates/react): add webpack entries to all templates Oct 1, 2020
@vanbasten17 vanbasten17 requested review from MarcosCA and guinii October 1, 2020 09:56
Copy link
Contributor

@asastre asastre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this improvement ❤️ 👏!

@vanbasten17 vanbasten17 force-pushed the fix/add-webchat-entry branch from 0f7618c to b86455d Compare October 1, 2020 10:38
@vanbasten17 vanbasten17 merged commit 8436fa5 into master Oct 1, 2020
@vanbasten17 vanbasten17 deleted the fix/add-webchat-entry branch October 1, 2020 10:50
@dpinol dpinol restored the fix/add-webchat-entry branch October 1, 2020 20:30
@dpinol dpinol deleted the fix/add-webchat-entry branch October 8, 2020 12:12
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.

4 participants