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

Add exception to transpile normalize-url mode to es5 when bundled into the DLL #35804

Merged
merged 6 commits into from May 2, 2019

Conversation

Projects
None yet
6 participants
@mistic
Copy link
Member

commented Apr 30, 2019

That PR closes #35759

Basically git-url-parse was updated to 11.1.2 which in dependencies of dependencies updates ends up with a normalize-url@3.0.3 in the browser. That dependency is a node_module that is not distributed with ES5 so unsupported ES6 code endups inside the DLL which causes troubles in IE 11.

@mistic mistic requested a review from spalger Apr 30, 2019

@mistic mistic self-assigned this Apr 30, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 30, 2019

@joshdover

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Is git-url-parse used in the browser? If so, is it going to break if it gets this older version of normalize-path? Maybe we should change the babel config instead to include transpiling this specific module.

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 30, 2019

@mistic

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

@joshdover the chain of dependencies is git-url-parse#git-up#parse-url I've looked to parse-url and the dependency bump happens in version 3.x (IonicaBizau/parse-url@3.0.2...master) and it looks to me that it was just a simple version upgrade. I think from normalize-url side they do not transpile the dist files to ES5 because it was a library intended to be used on node and not in the browser and they are also only supporting node 8+ right now. I would rather prefer to apply the yarn resolution or trying to work in the upstream dependencies in order to solve this than changing our babel configurations, because in general overview I think we should not transpile node_modules at all, if endup having that need we should avoid such dependencies as they are not targeting to our legacy needs. What do u think @spalger ?

@tylersmalley tylersmalley added the :Code label Apr 30, 2019

@tylersmalley

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@elastic/code is it possible to swap this package out with one that supports the browser? Or, could we remove our usage of this in the browser, maybe return the parsed URL in an API call?

@zfy0701

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

@elastic/code is it possible to swap this package out with one that supports the browser? Or, could we remove our usage of this in the browser, maybe return the parsed URL in an API call?

sounds good to me, how urgent is this?

@mistic

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

@zfy0701 that is a blocker for 7.1.0 feature freezing on May 7th so I think it is urgent 😃

@mistic

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

@zfy0701 I tried solve the problem in the upstream dependencies but that won't be possible as the maintainers didn't want to transpile their libraries before the npm publish step. That way I think we only have 2 solutions:

  • replace the library for other
  • return the parsed URL in an API call as @tylersmalley suggested

We can also keep the resolution for now to unblock, but I think we should really fix that quickly for the long term.

@zfy0701

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

it's not that easy to make them all as server side call because we parse url a lot in client

we used to use regex to do a lot of the url parsing but during the review process we switch to the same library with server, at the time we think it's valid suggestion, without any knowledge of this issue.

so option 1, swap out the package would mean revert
https://github.com/elastic/kibana/pull/33919/files#diff-075ffa1b96d97d491142b6ee627eacb5L13

we can try to find a replacement but I don't feel they are very promising.

option 2 would be keep this PR

option 3 would be fork this package and put it as @elastic/git-url-parse, so we can deal with the dependency issues in this package

I'm leaning towards 2 or 3, I would hear @spalger's opinion

@mistic

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Well over that info I would say that the best option would be trying to found something else to use than this library, so option 1.

In case that is not easy to do, I would say we should follow option 3.

@spalger

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Assuming all that would be necessary is transpiling this one module, I'm good with adding something like https://gist.github.com/spalger/af3dd51b25ffcb259624558136ced757 to the optimizer config as a one-time exception. The opinions expressed in sindresorhus/ama#446 resonate with me. That said, if this turns out to be the tip of an iceberg and tons of other modules are going to start doing the same thing, I think we would need a more appropriate solution.

Hopefully we can drop support for IE11 or get fully transitioned to the new platform if that future is headed for us soon.

I feel like it's fine to start with a little exception for now.

@mistic

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Let's move on this one. I would prefer other solutions to avoid loosing ends but as far as this is an exception and not the rule until we drop the support for IE11 I'm fine with it. I will update the PR @spalger @zfy0701

@mistic mistic changed the title chore(NA): add resolution to prevent unsupported normalize-url path. Add exception to transpile normalize-url mode to es5 when bundled into the DLL May 1, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented May 1, 2019

@mistic mistic added the v7.0.2 label May 1, 2019

mistic added some commits May 1, 2019

@mistic mistic removed the v7.0.2 label May 1, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented May 1, 2019

@tylersmalley
Copy link
Member

left a comment

LGTM and verified in IE

image

@mistic mistic merged commit 6c8f019 into elastic:master May 2, 2019

2 checks passed

CLA All commits in pull request signed
Details
kibana-ci Build finished.
Details

mistic added a commit to mistic/kibana that referenced this pull request May 2, 2019

Add exception to transpile normalize-url mode to es5 when bundled int…
…o the DLL (elastic#35804)

* chore(NA): add resolution to prevent unsupported normalize-url path.

* chore(NA): resolutions for create package json task.

* chore(NA): extend dll compiler config with normalize url module.

* chore(NA): add exclusion for normalize-url node_modules.
@mistic

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

7.x: 517a07c

mistic added a commit that referenced this pull request May 2, 2019

Add exception to transpile normalize-url mode to es5 when bundled int…
…o the DLL (#35804) (#35956)

* chore(NA): add resolution to prevent unsupported normalize-url path.

* chore(NA): resolutions for create package json task.

* chore(NA): extend dll compiler config with normalize url module.

* chore(NA): add exclusion for normalize-url node_modules.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.