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

fix: Provide es5 build for esm #1991

Merged
merged 1 commit into from
Apr 3, 2019
Merged

fix: Provide es5 build for esm #1991

merged 1 commit into from
Apr 3, 2019

Conversation

HazAT
Copy link
Member

@HazAT HazAT commented Apr 3, 2019

Fixes #1989
Fixes #1988
Fixes #1984
Fixes #1979

@HazAT HazAT self-assigned this Apr 3, 2019
@HazAT HazAT requested a review from kamilogorek as a code owner April 3, 2019 10:35
@HazAT
Copy link
Member Author

HazAT commented Apr 3, 2019

cc @cromefire @vkrol

@getsentry-bot
Copy link
Contributor

Messages
📖 ✅ TSLint passed
📖

@sentry/browser bundle gzip'ed minified size: (ES6: 14.2783 kB) (ES5: 15.6152 kB)

Generated by 🚫 dangerJS against 43f0f44

@cromefire
Copy link
Contributor

But in this case it's hard/impossible to opt into es2015+

@cromefire
Copy link
Contributor

It's up to the bundler, if it decides to select es2015 that is an issue of the bundler an not of Sentry. In the proposal I made esm + es5 is the default for module

@cromefire
Copy link
Contributor

I looked around and found that for webpack you can set this to disable importing of es2015:

module.exports = {
    //...
    resolve: {
        mainFields: ['module', 'main']
    }
};

@vkrol
Copy link
Contributor

vkrol commented Apr 3, 2019

@cromefire
As far as I know, webpack does not handle es2015 field in package.json.

@cromefire
Copy link
Contributor

@cromefire
Copy link
Contributor

I skimmed the code and it seems to ignore it, but the opposite works:

module.exports = {
    //...
    resolve: {
        mainFields: ["es2015", "browser", 'module', 'main']
    }
};

@vkrol
Copy link
Contributor

vkrol commented Apr 3, 2019

@cromefire Yes, webpack ignores this field and we need to configure them manually.

@cromefire
Copy link
Contributor

It would be good if this gets added to the documentation

@vkrol
Copy link
Contributor

vkrol commented Apr 3, 2019

@cromefire webpack documentation or Sentry documentation?

@cromefire
Copy link
Contributor

It's in the webpack dos, but quite hidden, so the sentry docs, because the specific field es2015 is specific to apf.

Something like:

If you want webpack to use the modern es2015+ version of sentry add the following to you configuration:

module.exports = {
    //...
    resolve: {
        mainFields: ["es2015", "browser", 'module', 'main']
    }
};

Also see: webpack documentation

@HazAT
Copy link
Member Author

HazAT commented Apr 3, 2019

Yes, @cromefire I found this too but I am still confused now

So, we will do:

module = es5 syntax with module imports (esm)
es2015 = es2015 syntax with module imports (esm)

right?

@HazAT
Copy link
Member Author

HazAT commented Apr 3, 2019

I decided not to emit es2015 yet since it's only for angular, and angular also works with just module if it's es5.

@HazAT HazAT merged commit 24d13cf into master Apr 3, 2019
@HazAT HazAT deleted the fix/esm branch April 3, 2019 16:17
@cromefire
Copy link
Contributor

es5 works but it's es5, so it's deprecated since 4 Years

@cromefire
Copy link
Contributor

I'm preparing a PR that implements everything

@cromefire
Copy link
Contributor

Yes, @cromefire I found this too but I am still confused now

So, we will do:

module = es5 syntax with module imports (esm)
es2015 = es2015 syntax with module imports (esm)

right?

Yes

@HazAT
Copy link
Member Author

HazAT commented Apr 3, 2019

@cromefire I will still release 5.0.5 now since it's unbreaking builds, we can take our time then to fix everything properly and provide all possible builds

@vkrol
Copy link
Contributor

vkrol commented Apr 3, 2019

@cromefire what do you think about this?

This works for ES2015, but it begs the question of what we should do about ES2016? Are we supposed to create a new folder for each year and a new field in package.json? That seems unsustainable, and will continue to produce larger node_modules.

https://babeljs.io/blog/2018/06/26/on-consuming-and-publishing-es2015+-packages#non-scalable-solutions

@cromefire
Copy link
Contributor

Well right now the es2015 builds are es2015+ right now

@vkrol
Copy link
Contributor

vkrol commented Apr 3, 2019

@cromefire please, can you elaborate more on that?

@cromefire
Copy link
Contributor

They are 1:1 the source code, so what ever the source is, is the result

@cromefire
Copy link
Contributor

That means that you are in charge of what will be in the output and have to look that your code runs in the latest browsers

@cromefire
Copy link
Contributor

https://babeljs.io/blog/2018/06/26/on-consuming-and-publishing-es2015+-packages#non-scalable-solutions

We could also publish it under esnext, rather than es2015

@cromefire
Copy link
Contributor

If got some basic builds working with only minor problems

@cromefire
Copy link
Contributor

For today just for @sentry/browser

@cromefire
Copy link
Contributor

Because of the utils I also have to get my rewrite script up and running again

@HazAT
Copy link
Member Author

HazAT commented Apr 4, 2019

@cromefire I don't want to have a rewrite script, we have to solve it differently even if it means to build utils different

@cromefire
Copy link
Contributor

Then index cannot have deep imports

@HazAT
Copy link
Member Author

HazAT commented Apr 4, 2019

I know, but we expose a index then in utils to fix this

@cromefire
Copy link
Contributor

Indeed that would be the solution, but this may make the bundle larger depending on tree shaking

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.

5 participants