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

build: use rollup bundler #335

Merged
merged 9 commits into from
Sep 7, 2020
Merged

build: use rollup bundler #335

merged 9 commits into from
Sep 7, 2020

Conversation

shikaan
Copy link
Contributor

@shikaan shikaan commented Aug 28, 2020

Purpose of PR

After getting this contentful/create-contentful-app#15 we have realized the reason for getting that warning is due to how Parcel works internally.

Given that Parcel doesn't provide clear guidance about how to deal with the problem, this PR migrates to another build tool.

Other considered options

  • esbuild
    Configuring esbuild was super easy, incredibly quick and the bundle is eventually smaller. The problem with that is that it is considered an "existence proof", so I wasn't sure to which extent it could be trusted;

Tested cases

To make sure this change is not breaking I have replicated locally the cases we expect to have:

  • requiring the package
    This what bundlers will do and it's been replicated by npm linking the package (this is what module in package.json is for)
  • using a <script>
    For this I wanted to replicate the behavior of importing the package from unpkg (as we do in the ready-made extensions). According to the documentation, links to libraries will point to whatever is the target of main. Hence, in order to test this, I have run a local server which was serving the main file and it was working.

Remarks

  • I have preferred the esbuild experience over this, but it's a risk which I am not sure we want to take;
  • We might consider waiting for Parcel or Webpack to fix the issue (and close this), if we don't feel like touching the build;

@shikaan shikaan marked this pull request as draft August 28, 2020 16:25
.nvmrc Outdated
v13.8.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use a non-dev version of node?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, would be nice use LTS

@davidfateh davidfateh changed the title build: [EXT-2116] use rollup bundler build: use rollup bundler Aug 31, 2020
@shikaan shikaan marked this pull request as ready for review August 31, 2020 15:33
.nvmrc Outdated
v13.8.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, would be nice use LTS

package.json Outdated Show resolved Hide resolved
@kdamball kdamball merged commit b34c625 into master Sep 7, 2020
@kdamball kdamball deleted the fix/EXT-2116-warning branch September 7, 2020 09:55
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