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(browser): build a dedicated version for the browser #242

Merged
merged 5 commits into from
May 15, 2018

Conversation

armandabric
Copy link
Collaborator

@armandabric armandabric commented Nov 10, 2017

Fix #211

This PR introduce a bundled version of the lib on NPM. Now the project is distributed on two forms:

  • dist/esm/: contains the same code has the previous one distributed on NPM. This code is designed to be used on with node and babel (we set the module entry in the package.json)
  • dist/cjs/: contains a bundled file that can be run in any ES2015 environment. Library users could have to polyfill their environment to make it ES2015 compliant. This bundled version is tagged as the default one and browser one in package.json

As the new build version became the default one. We will need to release a beta version to ensure everything works fine in most case.

@OriR
Copy link

OriR commented Nov 10, 2017

@Spy-Seth Thanks for addressing this so quickly!

I see that the output bundle contains parts of react code.
This means that in case I upgrade my version of react that has changes in these code parts react-element-to-jsx-string won't use the newer version but rather the contained one.
This probably applies to all other dependencies, while the problem was only with a specific one.

Since I'm not familiar with rollup that much, is there a way to define an external dependency in rollup like there there is in webpack?
Or if there's something similar to babel-engine-plugin for rollup so that only the required libraries are actually inlined for the cjs build.

That way you could just define the packages you depend on and the package consumer would have to supply them, making sure there's less code duplication and is no longer version dependent but rather API dependent.

@armandabric
Copy link
Collaborator Author

You right, I've forgotten to mark react, and react-dom has an external dependencies. I will add it.

I not sure of my choice of rollup. I find it more efficient for library bundling. But so few people know him... Maybe webpack could be a more reasonable tool.

@armandabric armandabric force-pushed the bundle-with-rollup branch 2 times, most recently from 3bc6f73 to 6223e4c Compare November 12, 2017 21:01
@algolia algolia deleted a comment Nov 12, 2017
@armandabric armandabric force-pushed the bundle-with-rollup branch 2 times, most recently from 94e0a63 to 5b3a750 Compare November 13, 2017 08:53
@armandabric
Copy link
Collaborator Author

It's lighter without react and react-dom inside it 😜

$ ls -lh dist/cjs/
total 56
-rw-r--r--  1 spy-seth  staff    22K Nov 13 09:57 bundle.js
-rw-r--r--  1 spy-seth  staff   823B Nov 13 09:57 bundle.js.map

@vvo
Copy link
Contributor

vvo commented Nov 13, 2017

Looks good, any blog post or article explaining all the reasoning behind the various build endpoints in package.json?

@armandabric
Copy link
Collaborator Author

You could have a look onto the rollup wiki for the module entry: https://github.com/rollup/rollup/wiki/pkg.module

For the browser one, there is a reference on it in rollup-plugin-node-resolve readme

By re-reading the rollup-plugin-node-resolve readme, I think I have made a mistake and we should target the esm build here and not the cjs one. Could someone give me a second thought on this?

      // some package.json files have a `browser` field which
      // specifies alternative files to load for people bundling
      // for the browser. If that's you, use this option, otherwise
      // pkg.browser will be ignored
      browser: true,  // Default: false

@vvo
Copy link
Contributor

vvo commented Nov 13, 2017

maybe @Haroenv can help?

@Haroenv
Copy link

Haroenv commented Nov 13, 2017

Seems good! What do you need help with Vincent?

@vvo
Copy link
Contributor

vvo commented Nov 16, 2017

On this message #242 (comment), both you and @samouss have rollup experience and might help @Spy-Seth here

Copy link

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

LGTM

@armandabric
Copy link
Collaborator Author

armandabric commented Nov 16, 2017

Are we confident on this change to directly do a major release. Or should we make some RC before?

@armandabric
Copy link
Collaborator Author

I just fix a typo in the first commit message

@vvo
Copy link
Contributor

vvo commented Nov 16, 2017

Do a major, we don't have any community nowaday to ask to test that for an RC I guess. What do you think?

@vvo
Copy link
Contributor

vvo commented Nov 16, 2017

You could always test it on your own project and the person having the browser need could also test it. If you release it under a different tag.

@armandabric
Copy link
Collaborator Author

I will try to find some time this weekend for some test and we could release it in the next week 😃

@briandipalma
Copy link

Any chance this could be merged without adding tests?

BREAKING CHANGE: This PR change of the internal directory structure of the exported code. The previous code has move from the `dist/` into the `dist/esm` directory (but remender that we do not avice you to do use internals code  🤓)
@ghost

This comment has been minimized.

@armandabric
Copy link
Collaborator Author

@vvo & @Haroenv All green and rebased :)

Sorry for the time to finish this

@Haroenv Haroenv requested a review from vvo May 9, 2018 13:56
@Haroenv
Copy link

Haroenv commented May 9, 2018

Thanks for taking the time @Spy-Seth, Vincent is currently off, but I'll remind him to review this when he's back

@vvo
Copy link
Contributor

vvo commented May 15, 2018

LGTM and you can release. Awesome work!!

@vvo vvo merged commit 574d850 into algolia:master May 15, 2018
@armandabric armandabric deleted the bundle-with-rollup branch May 16, 2018 13:04
@danoc
Copy link
Contributor

danoc commented May 24, 2018

Hello! Would love to use this to make my site work in IE 11.

Anything I can do help it get released?

@briandipalma
Copy link

I think we're just waiting for it to be published.

@vvo
Copy link
Contributor

vvo commented May 25, 2018

@Spy-Seth Can I release the lib safely? Does it requires a major?

@armandabric
Copy link
Collaborator Author

Yes we could release it a major :)

@vvo
Copy link
Contributor

vvo commented May 25, 2018

Ok doing it

@pascalduez pascalduez mentioned this pull request May 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants