Skip to content

Conversation

Delagen
Copy link
Contributor

@Delagen Delagen commented Aug 21, 2018

Purpose / Goal

Simple make split bundles with entries in webpack.config.js
Use latest features of ECMAScript language without any hacks

Type

Please mention the type of PR

[ ]Bug Fix
[x]Refactoring / Technology upgrade
[ ]New Feature

Note : Please ensure that you've read contribution guidelines before raising this PR.

@coveralls
Copy link

coveralls commented Aug 21, 2018

Coverage Status

Coverage remained the same at 97.487% when pulling 5f40401 on Delagen:webpack into e28cc29 on NaturalIntelligence:master.

@Delagen
Copy link
Contributor Author

Delagen commented Aug 21, 2018

I think to remove outdated node versions. All versions before 8 LTS reach EOL )

@amitguptagwl
Copy link
Member

Thanks @Delagen . Though moving to webpack will enable us splitting this package in small browser bundles, using ECMAScript latest syntax in the code etc, I wanted to check few things before merging this PR

  1. Need of generating parser.node.js.
  2. Need of pointing main script to lib/parser.node.js.
  3. Impact on typescript definitions.
  4. impact on vscode debugging

@Delagen
Copy link
Contributor Author

Delagen commented Aug 21, 2018

  1. Cause NodeJS can miss some function babel can provide
  2. Because of 1
  3. No impact
  4. Don't know

@amitguptagwl
Copy link
Member

Cause NodeJS can miss some function babel can provide

For example? are we using any such function? As this module is pretty stable I see less features to be implemented in the future.

@Delagen
Copy link
Contributor Author

Delagen commented Aug 21, 2018

For example ES2015 modules, async/await by default or something else... Need to check tables

@amitguptagwl
Copy link
Member

async/await is supported by node js. And we're not using that in our module anywhere. Can you please share some reference link where I can check those features? I couldn't find it through google.

@Delagen
Copy link
Contributor Author

Delagen commented Aug 21, 2018

https://node.green/

@Delagen
Copy link
Contributor Author

Delagen commented Aug 21, 2018

Many browser modules use ES2015 module model for efficient tree shaking

@Delagen
Copy link
Contributor Author

Delagen commented Aug 21, 2018

I don't think that NodeJS bundle really needed at this time, but creating it can offer support more older versions, but with lack of some travis testing, or we can make some tweaks )

@Delagen
Copy link
Contributor Author

Delagen commented Aug 21, 2018

I you argue for removing it, I can fix it )

@amitguptagwl
Copy link
Member

amitguptagwl commented Aug 22, 2018

@Delagen With my own experience and under the guidance of my supervisors I've learnt that Plan for future but code for current . So yes please remove the part that is not expected to be used soon.

@Delagen
Copy link
Contributor Author

Delagen commented Aug 22, 2018

Current is different for someone ) Some enterprise projects can use 0.12.x version or even older )
So should I remove this node bundle?

@amitguptagwl
Copy link
Member

Yes please

- 7
- 8
- 9
- 10
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason to remove 7, 9?

Copy link
Member

@amitguptagwl amitguptagwl left a comment

Choose a reason for hiding this comment

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

Is there any particular reason to remove node version 7 & 9 from travis configuration?

@Delagen
Copy link
Contributor Author

Delagen commented Aug 22, 2018

They are not supported any more https://github.com/nodejs/Release#release-schedule

@amitguptagwl amitguptagwl merged commit d4310a0 into NaturalIntelligence:master Aug 22, 2018
@Delagen
Copy link
Contributor Author

Delagen commented Aug 22, 2018

Then I think we need to create some entry points for building multiple bundles

@amitguptagwl
Copy link
Member

Yes. And I'm thinking to remove XML to Nimn transformation as Nimn seems duplicate of Google's protobuf.

@Zalastax
Copy link

This might be too late since it's already merged, but shouldn't rollup have been preferred over webpack, given that this is a library? Webpack has some overhead that rollup doesn't is my concern. It shouldn't be difficult to switch in the future though so might be best to keep with the flow you have going.

@amitguptagwl
Copy link
Member

amitguptagwl commented Aug 25, 2018

@Zalastax Thanks . It seems interesting. We can have a look. Raising an issue so it doesn't skip

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